You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2023/05/02 14:19:02 UTC
[GitHub] [maven] cstamas opened a new pull request, #1098: [MNG-7774] Maven config and command line interpolation (take two)
cstamas opened a new pull request, #1098:
URL: https://github.com/apache/maven/pull/1098
Reuse as much as possible from master, but keep existing stuff like multiModuleProjectDirectory alone.
Changes:
* interpolate user properties and arguments
* introduce session.topDirectory and session.rootDirectory
* Maven fails to start if any of the new properties are undefined but their use is attempted
---
https://issues.apache.org/jira/browse/MNG-7774
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] CrazyHZM commented on pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "CrazyHZM (via GitHub)" <gi...@apache.org>.
CrazyHZM commented on PR #1098:
URL: https://github.com/apache/maven/pull/1098#issuecomment-1534973768
@cstamas I think the description of adding this command in `mvn -help` is still missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1098:
URL: https://github.com/apache/maven/pull/1098#issuecomment-1534339709
> Does this actually supersede #1062?
No, that one is for master and builds on top of already present new mvn4 features not present in 3.9.x. This PR is for 3.9.x and "mildly follows" the one you refer to, with reduced functionalities. For example, that one uses `RootLocator` (already on master) while we have no such thing. Master also uses two strategies to identify "root", but it requires consumer POM feature, again not available here, etc. This PR provides "minimum viable change" to provide in 3.9.x:
* config and command line interpolation
* two new properties (but for interpolation ONLY), unlike on master, where these two bubble up to session and project
* the UT is intentionally 1:1 (so it IS copy pasted from referenced PR)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183386394
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
Well, I remember the interface https://github.com/codehaus-plexus/plexus-interpolation/blob/master/src/main/java/org/codehaus/plexus/interpolation/Interpolator.java#L24 :smile:
~But not the two imple differences (maybe regexp is even much newer)~. Hm, even the iface refers to "existing regexp"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183365054
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
As I see in maven codebase use of StringSearch vs Regexp interpolator is 4:5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1184010397
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
For me (and in light of use case), the simplest interpolation works, and out of two, the String one looks much much simpler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183348436
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -140,9 +146,14 @@ public class MavenCli {
private static final String EXT_CLASS_PATH = "maven.ext.class.path";
- private static final String EXTENSIONS_FILENAME = ".mvn/extensions.xml";
+ private static final String DOT_MVN = ".mvn";
- private static final String MVN_MAVEN_CONFIG = ".mvn/maven.config";
+ private static final String UNABLE_TO_FIND_ROOT_PROJECT_MESSAGE = "Unable to find the root directory. Create a "
+ + DOT_MVN + " directory in the project root directory to identify it.";
+
+ private static final String EXTENSIONS_FILENAME = DOT_MVN + "/extensions.xml";
Review Comment:
Shit, you are right. It just popped my eye.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183330513
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -140,9 +146,14 @@ public class MavenCli {
private static final String EXT_CLASS_PATH = "maven.ext.class.path";
- private static final String EXTENSIONS_FILENAME = ".mvn/extensions.xml";
+ private static final String DOT_MVN = ".mvn";
- private static final String MVN_MAVEN_CONFIG = ".mvn/maven.config";
+ private static final String UNABLE_TO_FIND_ROOT_PROJECT_MESSAGE = "Unable to find the root directory. Create a "
+ + DOT_MVN + " directory in the project root directory to identify it.";
+
+ private static final String EXTENSIONS_FILENAME = DOT_MVN + "/extensions.xml";
Review Comment:
Before and after this PR the contents of `EXTENSIONS_FILENAME` is unchanged, so I take this as nitpicking :smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1185941475
##########
maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java:
##########
@@ -364,6 +369,38 @@ public void testVersionStringWithoutAnsi() throws Exception {
assertEquals(MessageUtils.stripAnsiCodes(versionOut), versionOut);
}
+ @Test
+ public void testPropertiesInterpolation() throws Exception {
+ // Arrange
+ CliRequest request = new CliRequest(
+ new String[] {
+ "-Dfoo=bar",
+ "-DvalFound=s${foo}i",
+ "-DvalNotFound=s${foz}i",
+ "-DvalRootDirectory=${session.rootDirectory}/.mvn/foo",
+ "-DvalTopDirectory=${session.topDirectory}/pom.xml",
Review Comment:
This I understand now. Hope they won't be misunderstood by users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183884537
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -140,9 +146,14 @@ public class MavenCli {
private static final String EXT_CLASS_PATH = "maven.ext.class.path";
- private static final String EXTENSIONS_FILENAME = ".mvn/extensions.xml";
+ private static final String DOT_MVN = ".mvn";
- private static final String MVN_MAVEN_CONFIG = ".mvn/maven.config";
+ private static final String UNABLE_TO_FIND_ROOT_PROJECT_MESSAGE = "Unable to find the root directory. Create a "
+ + DOT_MVN + " directory in the project root directory to identify it.";
+
+ private static final String EXTENSIONS_FILENAME = DOT_MVN + "/extensions.xml";
+
+ private static final String MVN_MAVEN_CONFIG = DOT_MVN + "/maven.config";
Review Comment:
Same solution as before (PR does not change anything)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183323362
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -140,9 +146,14 @@ public class MavenCli {
private static final String EXT_CLASS_PATH = "maven.ext.class.path";
- private static final String EXTENSIONS_FILENAME = ".mvn/extensions.xml";
+ private static final String DOT_MVN = ".mvn";
- private static final String MVN_MAVEN_CONFIG = ".mvn/maven.config";
+ private static final String UNABLE_TO_FIND_ROOT_PROJECT_MESSAGE = "Unable to find the root directory. Create a "
+ + DOT_MVN + " directory in the project root directory to identify it.";
+
+ private static final String EXTENSIONS_FILENAME = DOT_MVN + "/extensions.xml";
+
+ private static final String MVN_MAVEN_CONFIG = DOT_MVN + "/maven.config";
Review Comment:
Suffers from the same more or less.
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -140,9 +146,14 @@ public class MavenCli {
private static final String EXT_CLASS_PATH = "maven.ext.class.path";
- private static final String EXTENSIONS_FILENAME = ".mvn/extensions.xml";
+ private static final String DOT_MVN = ".mvn";
- private static final String MVN_MAVEN_CONFIG = ".mvn/maven.config";
+ private static final String UNABLE_TO_FIND_ROOT_PROJECT_MESSAGE = "Unable to find the root directory. Create a "
+ + DOT_MVN + " directory in the project root directory to identify it.";
+
+ private static final String EXTENSIONS_FILENAME = DOT_MVN + "/extensions.xml";
Review Comment:
That's not a filename anymore, but a path.
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
Any reason not to use the `StringRegexInterpolator`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183386394
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
Well, I remember the interface https://github.com/codehaus-plexus/plexus-interpolation/blob/master/src/main/java/org/codehaus/plexus/interpolation/Interpolator.java#L24 :smile:
But not the two imple differences (maybe regexp is even much newer)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183331161
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
The interpolator thing is pretty much coming from ~master~ https://github.com/apache/maven/pull/1062. What is the difference?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1184662324
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -309,6 +320,47 @@ void initialize(CliRequest cliRequest) throws ExitException {
}
}
+ // We need to locate the top level project which may be pointed at using
+ // the -f/--file option. However, the command line isn't parsed yet, so
+ // we need to iterate through the args to find it and act upon it.
+ Path topDirectory = Paths.get(cliRequest.workingDirectory);
+ boolean isAltFile = false;
+ for (String arg : cliRequest.args) {
+ if (isAltFile) {
+ // this is the argument following -f/--file
+ Path path = topDirectory.resolve(arg);
+ if (Files.isDirectory(path)) {
+ topDirectory = path;
+ } else if (Files.isRegularFile(path)) {
+ topDirectory = path.getParent();
+ if (!Files.isDirectory(topDirectory)) {
+ System.err.println("Directory " + topDirectory
+ + " extracted from the -f/--file command-line argument " + arg + " does not exist");
+ throw new ExitException(1);
+ }
+ } else {
+ System.err.println(
+ "POM file " + arg + " specified with the -f/--file command line argument does not exist");
+ throw new ExitException(1);
+ }
+ break;
+ } else {
+ // Check if this is the -f/--file option
+ isAltFile = arg.equals(String.valueOf(CLIManager.ALTERNATE_POM_FILE)) || arg.equals("file");
+ }
+ }
+ try {
+ topDirectory = topDirectory.toAbsolutePath().toRealPath();
+ } catch (IOException e) {
+ System.err.println("Error computing real path from " + topDirectory);
Review Comment:
Agreed, included IOEx message as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183386394
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
Well, I remember the interface https://github.com/codehaus-plexus/plexus-interpolation/blob/master/src/main/java/org/codehaus/plexus/interpolation/Interpolator.java#L24 :smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1185906988
##########
maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java:
##########
@@ -364,6 +369,38 @@ public void testVersionStringWithoutAnsi() throws Exception {
assertEquals(MessageUtils.stripAnsiCodes(versionOut), versionOut);
}
+ @Test
+ public void testPropertiesInterpolation() throws Exception {
+ // Arrange
+ CliRequest request = new CliRequest(
+ new String[] {
+ "-Dfoo=bar",
+ "-DvalFound=s${foo}i",
+ "-DvalNotFound=s${foz}i",
+ "-DvalRootDirectory=${session.rootDirectory}/.mvn/foo",
+ "-DvalTopDirectory=${session.topDirectory}/pom.xml",
Review Comment:
Expressions HAVE BEEN (in this PR) backported, what you *really mean* is that 3.9.x does not expose them on MavenSession object, that is true. Still, the point of this PR is to make builds between 3.9.x and 4 permeable, "work with both". This PR as explained above, *does not back port everything*, just the ability to interpolate and these two "special" expressions used in interpolation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183348912
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
I remember that one has superseded the other, I don't exactly remember.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183365054
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
As I see in maven codebase use of StringSearch vs Regexp interpolator is 4:5, this PR (if merged as is) will make it 5:5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1098:
URL: https://github.com/apache/maven/pull/1098#issuecomment-1534979166
> @cstamas I think the description of adding this command in `mvn -help` is still missing.
Which command? No command was added or modified in Maven CLI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] CrazyHZM commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "CrazyHZM (via GitHub)" <gi...@apache.org>.
CrazyHZM commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1184555983
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -309,6 +320,47 @@ void initialize(CliRequest cliRequest) throws ExitException {
}
}
+ // We need to locate the top level project which may be pointed at using
+ // the -f/--file option. However, the command line isn't parsed yet, so
+ // we need to iterate through the args to find it and act upon it.
+ Path topDirectory = Paths.get(cliRequest.workingDirectory);
+ boolean isAltFile = false;
+ for (String arg : cliRequest.args) {
+ if (isAltFile) {
+ // this is the argument following -f/--file
+ Path path = topDirectory.resolve(arg);
+ if (Files.isDirectory(path)) {
+ topDirectory = path;
+ } else if (Files.isRegularFile(path)) {
+ topDirectory = path.getParent();
+ if (!Files.isDirectory(topDirectory)) {
+ System.err.println("Directory " + topDirectory
+ + " extracted from the -f/--file command-line argument " + arg + " does not exist");
+ throw new ExitException(1);
+ }
+ } else {
+ System.err.println(
+ "POM file " + arg + " specified with the -f/--file command line argument does not exist");
+ throw new ExitException(1);
+ }
+ break;
+ } else {
+ // Check if this is the -f/--file option
+ isAltFile = arg.equals(String.valueOf(CLIManager.ALTERNATE_POM_FILE)) || arg.equals("file");
+ }
+ }
+ try {
+ topDirectory = topDirectory.toAbsolutePath().toRealPath();
+ } catch (IOException e) {
+ System.err.println("Error computing real path from " + topDirectory);
Review Comment:
Would it be easier to troubleshoot the problem if the exception information was printed out here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1098:
URL: https://github.com/apache/maven/pull/1098#issuecomment-1534947001
Anyone else?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183379283
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
I wonder they there are two what the difference is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation (take two)
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1183331161
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -526,8 +578,39 @@ private void commands(CliRequest cliRequest) {
// Needed to make this method package visible to make writing a unit test possible
// Maybe it's better to move some of those methods to separate class (SoC).
- void properties(CliRequest cliRequest) {
- populateProperties(cliRequest.commandLine, cliRequest.systemProperties, cliRequest.userProperties);
+ void properties(CliRequest cliRequest) throws ExitException {
+ try {
+ populateProperties(cliRequest, cliRequest.systemProperties, cliRequest.userProperties);
+
+ StringSearchInterpolator interpolator =
Review Comment:
The interpolator thing is pretty much coming from master. What is the difference?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #1098:
URL: https://github.com/apache/maven/pull/1098#issuecomment-1534325920
Does this actually supersede https://github.com/apache/maven/pull/1062?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1098:
URL: https://github.com/apache/maven/pull/1098#discussion_r1185835588
##########
maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java:
##########
@@ -364,6 +369,38 @@ public void testVersionStringWithoutAnsi() throws Exception {
assertEquals(MessageUtils.stripAnsiCodes(versionOut), versionOut);
}
+ @Test
+ public void testPropertiesInterpolation() throws Exception {
+ // Arrange
+ CliRequest request = new CliRequest(
+ new String[] {
+ "-Dfoo=bar",
+ "-DvalFound=s${foo}i",
+ "-DvalNotFound=s${foz}i",
+ "-DvalRootDirectory=${session.rootDirectory}/.mvn/foo",
+ "-DvalTopDirectory=${session.topDirectory}/pom.xml",
Review Comment:
I have a problem with these two expressions: They haven't been back ported to 3.9.x, no? So they are fake here and not work throughout the entire build?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] CrazyHZM commented on pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "CrazyHZM (via GitHub)" <gi...@apache.org>.
CrazyHZM commented on PR #1098:
URL: https://github.com/apache/maven/pull/1098#issuecomment-1534997859
> > @cstamas I think the description of adding this command in `mvn -help` is still missing.
>
> Which command? No command was added or modified in Maven CLI
My mistake was that I didn't see it clearly. There was nothing wrong with 3.9.x. There are too many maven versions for my local development. 😢 @cstamas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas merged pull request #1098: [MNG-7774] Maven config and command line interpolation
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #1098:
URL: https://github.com/apache/maven/pull/1098
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org