Skip to content
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-5889 - adding logic that looks for the file argument and starts t… #94

Closed
wants to merge 1 commit into from

Conversation

@rpatrick00
Copy link

rpatrick00 commented Sep 15, 2016

…he search for the .mvn directory at the location of the specified POM when present

@Tibor17

This comment has been minimized.

Copy link
Contributor

Tibor17 commented Oct 9, 2016

@rpatrick00
It looks like "--file" is not recognize but "-file" is recognized.
The second issue where I have /a/b folders and b contains single pom.xml and .mvn - no multimodule.
cd /a which does not have .mvn and start mvn -file b/pom.xml. There my vm.config is not applied, why? Tested on Win. I will try in Fedora as well.

@rpatrick00

This comment has been minimized.

Copy link
Author

rpatrick00 commented Oct 9, 2016

OK, I found and resolved the problem with the --file arg but am not seeing the other problem you describe (see output below). Maybe my fix for the --file arg resolved that one too?

Here is a stupid little test project that I am using:
maven-test.zip

In the root directory of the project, there is a git.diff file that shows the changes I made to the pull request code. I am happy to resubmit the pull request with this change but would prefer to verify that it fixes the issues in your Windows environment first.

If you run with JDK 8, you will see the warning about ignoring MaxPermSize=128m, which is set in jvm.config:

D:\temp>mvn --file maven-test\pom.xml verify
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=128m; support was removed in 8.0
[INFO] Scanning for projects...
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Building maven-config-test 1.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO]
[INFO] --- echo-maven-plugin:0.3.0:echo (default) @ maven-config-test ---
[INFO]
[INFO]
[INFO]
[INFO]
[INFO] myproperty = from.maven.config
[INFO]
[INFO]
[INFO]
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 0.587 s
[INFO] Finished at: 2016-10-09T11:20:56-05:00
[INFO] Final Memory: 17M/491M
[INFO] ------------------------------------------------------------------------

@Tibor17

This comment has been minimized.

Copy link
Contributor

Tibor17 commented Oct 9, 2016

Don't create a new PR. Just amend the last commit git commit --amend and push it again.

@rpatrick00

This comment has been minimized.

Copy link
Author

rpatrick00 commented Oct 9, 2016

Pushed

@Tibor17

This comment has been minimized.

Copy link
Contributor

Tibor17 commented Nov 2, 2016

@rpatrick00
LGTM
I have tested the change on Win7 and Fedora23.
I will merge your PR with master.

goto findBaseDir
)
call :get_directory_from_file %FILE_ARG%
if not exist "%POM_DIR%" (

This comment has been minimized.

Copy link
@michael-o

michael-o Nov 5, 2016

Member

Why is this check not present in the shell script?

This comment has been minimized.

Copy link
@rpatrick00

rpatrick00 Nov 5, 2016

Author

Like it or not, sh and cmd are different and have different capabilities. In the sh script, we know that the directory exists because of the check for existence of the POM file. I easily can add the POM file existence test to the cmd script and print the same errors if the pom file does not exist.

In the cmd script, I need to check for the existence of the extracted directory because if, for some reason, the directory extracted doesn't exist (e.g., a bug in the extraction code), the user gets a cryptic error. The same is not true for the sh script.

I will add the redundant check to the sh script too just for symmetry purposes...

)
call :get_directory_from_file %FILE_ARG%
if not exist "%POM_DIR%" (
echo Directory %POM_DIR% extracted from the -f/--file command-line argument does not exist

This comment has been minimized.

Copy link
@michael-o

michael-o Nov 5, 2016

Member

This should go to stderr.

This comment has been minimized.

Copy link
@rpatrick00
@rpatrick00

This comment has been minimized.

Copy link
Author

rpatrick00 commented Nov 6, 2016

OK, addressed both comments from michael-o

…he search for the .mvn directory at the location of the specified POM when present
@Tibor17

This comment has been minimized.

Copy link
Contributor

Tibor17 commented Nov 12, 2016

@michael-o
Any objections to merge this or do we need to test it with specific use case?

@michael-o

This comment has been minimized.

Copy link
Member

michael-o commented Nov 12, 2016

I think this really deserves an IT too. The shell script is fine. Can't just on the command script, I am virtually illiterate here. I trust @rpatrick00 here. Willing to merge blindly when the IT is ready.

@Tibor17

This comment has been minimized.

Copy link
Contributor

Tibor17 commented Nov 12, 2016

@michael-o
I will merge it no problem. The only problem on IT is my spare time.

@michael-o

This comment has been minimized.

Copy link
Member

michael-o commented Nov 12, 2016

I would rather see @rpatrick00 providing the IT.

@rpatrick00

This comment has been minimized.

Copy link
Author

rpatrick00 commented Nov 13, 2016

@michael-o, while I have no problem contributing, I have no familiarity with the maven-integration-test project so it will likely take me some time to reverse engineer it enough to figure out how to create an integration test for the shell scripts and the various use cases that should be tested. I would hate to see this fix gated by the requirement for me to accomplish this given the demands of my day job...

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 13, 2016

let's help: I dug into core-its, to find what existing IT to extend, since .mvn feature should have an IT already
=> found mng-5771-core-extensions IT, which already tests 3 conditions: I think it's the right place to add one more
here are the pointers:

I suppose writing the IT will simply require to copy testCoreExtension() method and not setting basedir on the Verifier, but add the new option

here is core-its intro: http://maven.apache.org/core-its/core-it-suite/

do you want to try? Or I do it (now that I had a look and found existing IT, seems easy)

@michael-o

This comment has been minimized.

Copy link
Member

michael-o commented Nov 13, 2016

@hboutemy Thanks for helping out!

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 13, 2016

I know core ITs are intimidating: it took me years to really dig into the code!
This case seems a simple one (what I didn't really expect when thinking at it initially), since it's only improving existing MNG-5771 IT with just different launch conditions

if it is still too much intimidating, tell me: I'll do the IT showing the failure, and you'll add the fix :)

@rpatrick00

This comment has been minimized.

Copy link
Author

rpatrick00 commented Nov 13, 2016

I took a quick look at the MNG-5771 IT and it's not obvious to me exactly what I need to do so if you can add this in a few minutes, that is probably the best thing to do. If not, I will try to look at it as I can and try some tests to figure out how it works...

@jvanzyl

This comment has been minimized.

Copy link
Contributor

jvanzyl commented Nov 13, 2016

Please do not change an existing test. The conditions under which this IT passes should be left as-is and should continue running as it is in exactly the form it runs now. Making purely additional logic is fine, or copy that IT and make another test all together, do not change the logic or resources of the existing IT. It captures behavior that is currently deployed in released versions and so it should remain as a representation of that.

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 13, 2016

IT added apache/maven-integration-testing@2e74409
no change on existing ITs, just added an additional run with different execution state: current dir = parent, mvn run with -f client/pom.xml
of course, it is currently failing: I let Tibor or Michael apply the fix

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 14, 2016

IT moved to its own suite in apache/maven-integration-testing@3311f4e to avoid running this IT with Maven 3.3.x, as it will fail

@michael-o

This comment has been minimized.

Copy link
Member

michael-o commented Nov 14, 2016

@hboutemy I am bit confused by the IT. The issue was about having .mvn properly located to expand maven.config. I would expect some system property to be added and retrieved. Am I wrong?

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 14, 2016

maybe I'm missing something...
.mvn directory has to be found by shell script because of jvm.config particularly
But once he directory is found by shell script, I suppose it is injected in JVM launch command to be used for every other file (maven.config and extensions.xml): that's perhaps where I'm wrong, I didn't check
I just looked at Linux shell update, that properly finds jvm.config: and I supposed that the IT checking for extensions.xml was consistent.
Perhaps this is just a beginning and has to be improved to check more that simply extensions.xml is loaded: adding more tests and conditions to this IT should not be hard (will just require this time to copy the files from orinig mng 5771 IT)

@michael-o

This comment has been minimized.

Copy link
Member

michael-o commented Nov 14, 2016

@rpatrick00 Can you shed some light here? What do you to properly picked up? extensions.xml, maven.config, and jvm.config, right?

@rpatrick00

This comment has been minimized.

Copy link
Author

rpatrick00 commented Nov 14, 2016

If you look at the shell script, it used to compute the "basedir" variable starting at the current working directory and then search upwards for the .mvn directory. Then, it set the MAVEN_PROJECTBASEDIR environment variable and added the contents of jvm.config. Finally, it passed the location into the java command line using the -Dmaven.multiModuleProjectDirectory=${MAVEN_PROJECTBASEDIR} argument.

All my fix does is change the starting point to search for the .mvn directory if the -f/--file argument is specified. The rest of the logic is exactly the same...

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 14, 2016

yes, that's exactly the part I was looking for. The shell script:

  1. finds the basedir
  2. uses it to load jvm.config
  3. then injects the value as maven.multiModuleProjectDirectory property for the java part to use it to load maven.config and extensions.xml

Then when @rpatrick00 improves the basedir discover algorithm, it works both for jvm.config, maven.config and extensions.xml

Ans when the IT checks that extensions.xml is loaded, it shows one of the features: perhaps we could add checks for the 2 others, but I admint that when extensions.xml works, the other work too
(in fact, I didn't find any IT for the 2 others, then adding a test could be useful to fix lack of previous IT...)

@michael-o

This comment has been minimized.

Copy link
Member

michael-o commented Nov 14, 2016

@hboutemy I would at least check for jvm.config because this is expanded in the script itself.

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 14, 2016

IMHO, the first thing is to check in the current fix, since ITs are currently broken
then improving the IT to check more parts could be interesting: priority 2

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 14, 2016

I just merged the patch
I had to change == tests to = since it did not work on Linux: the result is if [ "$arg" = "-f" -o "$arg" = "--file" ]; then
And it required a little update to the IT to force JVM fork, to force use of the shell script when launching IT build
Everything seems good: I hope ASF Jenkins will tell that it works like it worked on my machine :)

Thank you @rpatrick00 and Maven devs who helped him: this one is a great improvement

@rpatrick00

This comment has been minimized.

Copy link
Author

rpatrick00 commented Nov 14, 2016

@hboutemy, sorry but both "==" and "=" seem to work fine on Oracle Linux and MacOS (where I tested the sh script). Not sure which version of Linux caused this but thanks for fixing it...

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 14, 2016

FYI I'm running Kubuntu 16.10
not a big issue: thank your for the great job
please take time to close the PR

@rpatrick00 rpatrick00 closed this Nov 15, 2016
@michael-o

This comment has been minimized.

Copy link
Member

michael-o commented Nov 15, 2016

@hboutemy Are you waiting for the Jenkins build to close the JIRA issue? @rpatrick00 Thank you for the contribution.

@hboutemy

This comment has been minimized.

Copy link
Member

hboutemy commented Nov 15, 2016

yes: now that ASF Jenkins shows me it works not only on my Linux box but also on ASF Linux and on Windows, I just closed the Jira issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.