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

Use relative wrapper paths to prevent space-in-path bugs #7558

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Jul 10, 2024

The mvnw wrapper contains a subtle bug, which triggers when
-running on UNIXes, and

  • there's a space in the project path, and
  • wrapper is invoked using full path
    In that case, the wrapper fails to locate its basedir and loops indefinitely, not even launching Maven itself. Regular user just uses ./mvnw or ../mvnw from the terminal, and is not affected. Sadly NB uses full path to invoke the wrapper.

The bug was fixed an year ago in maven-wrapper, line 173 but since the wrapper script is committed in many git repositores, NB should work well even with the buggy version.

This PR relativizes the wrapper path to the execution directoy (either the project dir, or the reactor dir) which works around this bug: one would have to have spaces in path to maven module within the project, which is quite unlikely (and such project would not be usable even from the commandline).

@sdedic sdedic added kind:bug Bug report or fix Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Maven [ci] enable "build tools" tests VSCode Extension [ci] enable VSCode Extension tests labels Jul 10, 2024
@sdedic sdedic added this to the NB23 milestone Jul 10, 2024
@sdedic sdedic requested review from neilcsmith-net and dbalek July 10, 2024 10:37
@sdedic sdedic self-assigned this Jul 10, 2024
Copy link
Member

@neilcsmith-net neilcsmith-net left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

The script you point to is no longer even the default wrapper script. Have you checked before and after with the only-script variant too?

@sdedic sdedic requested a review from MartinBalin July 10, 2024 11:13
@sdedic
Copy link
Member Author

sdedic commented Jul 10, 2024

Re.: motivation for this PR - both Micronaut Launcher and GDK Launcher (Oracle's work) use an older mvnw to generate the projects, this PR allows NB to work well with those generated projects.

The script you point to is no longer even the default wrapper script. Have you checked before and after with the only-script variant too?

Pls. elaborate ... whta script should be used then ? I've tried to use mvn wrapper:wrapper (maven 3.9.8) and still mvnw in the project root seems to be the one.

So if this fix is partial, but working, I'd prefer to merge, and work on an improvement.

@sdedic
Copy link
Member Author

sdedic commented Jul 10, 2024

Ah, I understood after looking into the wrapper ;)
Yes, the new wrapper version that wget-downloads the distribution works even with the original MavenCommandLineExecutor and does not break with this PR.

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Jul 10, 2024

Yes, the default script is now https://github.com/apache/maven-wrapper/blob/master/maven-wrapper-distribution/src/resources/only-mvnw since apache/maven-wrapper#127 although still called mvnw in the project.

Thanks for checking!

@MartinBalin
Copy link
Contributor

Micronaut developer explained me that they tried new maven wrapper in June and it caused them some serious side effects and issues in eg guides generation. So they rolled back to older version...

@sdedic sdedic merged commit 846d426 into apache:master Jul 10, 2024
39 checks passed
@mbien
Copy link
Member

mbien commented Jul 10, 2024

thanks for fixing this. I is just a bit of a shame that workarounds like this one here will have to stick around for a very long time.

I think it would be better if IDEs would detect problematic wrapper scripts, notify the user and run wrapper:wrapper if the user gives the OK. This would remove the need for workarounds and also help users to fix issues in their repos - that is what IDEs are for IMO.

@sdedic
Copy link
Member Author

sdedic commented Jul 10, 2024

@mbien file a feature request for that pls... good idea !

@mbien
Copy link
Member

mbien commented Aug 14, 2024

@mbien file a feature request for that pls... good idea !

@sdedic I haven't forgotten about this, i wanted to bring this to the mailing list later between releases, but the recent discussions accelerated it a bit, so I wrote it down right away: https://lists.apache.org/thread/lgfvt0649rlg83j46bcczxj30y87qh95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) kind:bug Bug report or fix Maven [ci] enable "build tools" tests VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants