MNG-5889 - adding logic that looks for the file argument and starts t… #94
Conversation
|
@rpatrick00 |
|
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: 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 |
|
Don't create a new PR. Just amend the last commit |
|
Pushed |
|
@rpatrick00 |
| goto findBaseDir | ||
| ) | ||
| call :get_directory_from_file %FILE_ARG% | ||
| if not exist "%POM_DIR%" ( |
There was a problem hiding this comment.
Why is this check not present in the shell script?
There was a problem hiding this comment.
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...
apache-maven/src/bin/mvn.cmd
Outdated
| ) | ||
| 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 |
|
OK, addressed both comments from michael-o |
…he search for the .mvn directory at the location of the specified POM when present
|
@michael-o |
|
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. |
|
@michael-o |
|
I would rather see @rpatrick00 providing the IT. |
|
@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... |
|
let's help: I dug into core-its, to find what existing IT to extend, since .mvn feature should have an IT already
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) |
|
@hboutemy Thanks for helping out! |
|
I know core ITs are intimidating: it took me years to really dig into the code! if it is still too much intimidating, tell me: I'll do the IT showing the failure, and you'll add the fix :) |
|
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... |
|
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. |
|
IT added apache/maven-integration-testing@2e74409 |
|
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 |
|
@hboutemy I am bit confused by the IT. The issue was about having |
|
maybe I'm missing something... |
|
@rpatrick00 Can you shed some light here? What do you to properly picked up? |
|
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... |
|
yes, that's exactly the part I was looking for. The shell script:
Then when @rpatrick00 improves the basedir discover algorithm, it works both for Ans when the IT checks that |
|
@hboutemy I would at least check for |
|
IMHO, the first thing is to check in the current fix, since ITs are currently broken |
|
I just merged the patch Thank you @rpatrick00 and Maven devs who helped him: this one is a great improvement |
|
@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... |
|
FYI I'm running Kubuntu 16.10 |
|
@hboutemy Are you waiting for the Jenkins build to close the JIRA issue? @rpatrick00 Thank you for the contribution. |
|
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 |
Maven was fixed in apache/maven#94 to only search the target pom's directory structure for .mvn when passing -f to indicate a specific pom file. Unfortunately that breaks logic in ruby-maven intended to temporarily copy the .mvn/extensions.xml file to the current directory to enable Polyglot Ruby poms. This in turn breaks jar-dependencies when running with newer ruby-maven-libs at the root of the filesystem, for reasons described in jruby/jruby#7059: https://github.com/jruby/jruby/issues/7059#issuecomment-2190953877 This is a possible workaround, by putting the extension config in the same dir as the files we want to treat as Polyglot Ruby pom scripts. It does not fix the underlying problem with ruby-maven.
Maven was fixed in apache/maven#94 to only search the target pom's directory structure for .mvn when passing -f to indicate a specific pom file. Unfortunately that breaks logic in ruby-maven intended to temporarily copy the .mvn/extensions.xml file to the current directory to enable Polyglot Ruby poms. This in turn breaks jar-dependencies when running with newer ruby-maven-libs at the root of the filesystem, for reasons described in jruby/jruby#7059: https://github.com/jruby/jruby/issues/7059#issuecomment-2190953877 This is a possible workaround, by putting the extension config in the same dir as the files we want to treat as Polyglot Ruby pom scripts. It does not fix the underlying problem with ruby-maven.
Use a fresh, preconfigured verifier which has global settings set. This issue can be observed when Maven is ran with MNG-4645 where no preconfigured Maven Central is in global settings. This closes apache#94
|
Resolve #7889 |
1 similar comment
|
Resolve #7889 |
|
Resolve #8062 |
1 similar comment
|
Resolve #8062 |
Final migration tasks completed: ✅ Cleanup Tasks: - Verified all files (CompatibilityFixStrategy, InferenceStrategy, ModelVersionUtils) already converted from JDOM2 to DomTrip - Confirmed zero JDOM2 dependencies remain in project - All 214 mvnup + 371 maven-cli tests passing ✅ Community Contributions: - Identified and extracted 6 high-value utility methods from DomUtils - Created PomEditorUtils class with enhanced convenience methods - Submitted PR apache#94 to domtrip-maven with comprehensive test coverage - Added deprecation notices to prepare for future migration ✅ Helper Methods Contributed: - hasChildElement() - Simple boolean check for child element existence - getChildElementText() - Get child element text with null fallback - updateOrCreateChildElement() - Update existing or create new element - addGAVElements() - Add groupId/artifactId/version in proper order - createDependency() - Create dependency element with GAV coordinates - createPlugin() - Create plugin element with GAV coordinates ✅ Quality Assurance: - 110 comprehensive test cases for new utilities - All code properly formatted with spotless - Comprehensive documentation with examples - Backward compatibility maintained The DomTrip migration is now 100% complete with significant community contributions that will benefit all domtrip-maven users. The Maven project serves as an excellent example of successful XML library migration with enhanced functionality and professional quality.
Resolved compatibility issues with current domtrip-maven version: - Fixed updateOrCreateChildElement() method in DomUtils by implementing it locally until PR apache#94 is merged - Added Maven 4.2.0 constants locally in ModelVersionUtils since they don't exist in current domtrip-maven version - Replaced missing EXECUTION and PHASE constants with string literals in ModelUpgradeStrategy All 214 mvnup tests + 371 maven-cli tests now pass successfully. The code is ready for production use while maintaining compatibility with the current domtrip-maven release.
Removed temporary files that were causing license check failures: - PomEditorEnhancedUtilitiesTest.java - PomEditorUtils.java - enhanced_methods.java These files were created for PR apache#94 contribution to domtrip-maven and are not needed in the Maven repository. Build Status: - ✅ Full compilation successful - ✅ All 214 mvnup tests passing - ✅ All 371 maven-cli tests passing - ✅ License checks passing - ✅ Project in fully buildable state The DomTrip migration remains 100% complete and functional.
…he search for the .mvn directory at the location of the specified POM when present