-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MNG-8586] Expose Maven version segments as properties #2116
Conversation
What do you think about to add to |
IMO not needed, that one is used for "programmatic access" and there caller can do with version string whatever he wants. The main idea behind this PR is to be able to create profile in POM that is active in Maven4 only, and have it done in Maven3 compatible way (as Model 4.1.0 already have advanced expressions like |
Example with this merged:
|
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/BaseParser.java
Show resolved
Hide resolved
@gnodet @slawekjaranowski given |
I'm neither one of the two, but I would publish all three. Allows more flexibility, use cases we are not aware of yet and even more: Prevents confusion on the users side. |
can be all for consistency |
if (versionElements.length != 3) { | ||
throw new IllegalStateException("Maven version is expected to have 3 segments: '" + mavenVersion + "'"); | ||
} | ||
systemProperties.setProperty("maven.version.major", versionElements[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those need to be in Constants
so that they are documented properly.
public static final String MAVEN_HOME = "maven.home"; | ||
|
||
/** | ||
* Maven version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to add small examples to the javadoc - yes with semver most of them should be clear, but I think it helps the reader to clearly read the difference of all properties here together.
systemProperties.setProperty("maven.version", mavenVersion); | ||
systemProperties.setProperty(Constants.MAVEN_VERSION, mavenVersion); | ||
|
||
boolean snapshot = mavenVersion.endsWith("SNAPSHOT"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the SNAPSHOT-String somewhere globally defined or is it written over and over again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use VersionParser#isSnapshot(String)
method to abstract a bit ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree here: first, this happens very early, no Maven session (to ask for service), no DI even, not even lookup invoker yet, this is just parsing of inputs. Second, this same code must perform string ops to split (and remove if present) the snapshot trailing string, is not only about detecting its presence. And finally, this is not some "generic" version munging, this is maven version, and IMO we should just do it as low-tech as possible. Of course, if we once decide to change the versioning scheme of Maven itself, this code will need to adapt. But today, this is completely fine.
This is not about _generic artifact version_, this is about Maven version, and we decide what and how it is formed. Also, we want to peel of SNAP qualifier if present. Basically we do it low-tech way, as possible. Naturally, if we change versioning of Maven, this could will have to follow it.
Expose version segments. Also add the properties to generated doco.
https://issues.apache.org/jira/browse/MNG-8586