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

mvnd overwrites PATH for vanilla mvn #787

Closed
henningn opened this issue Feb 7, 2023 · 21 comments
Closed

mvnd overwrites PATH for vanilla mvn #787

henningn opened this issue Feb 7, 2023 · 21 comments

Comments

@henningn
Copy link

henningn commented Feb 7, 2023

I use sdkman and both maven (3.9.0) and mvnd (0.8.2, 0.9.0) are installed.
If mvnd 0.9.0 is set as default version, the command "mvn --version" shows the version 3.8.7.
If mvnd 0.8.2 is set as default version, the command "mvn --version" shows the version 3.9.0 (as expected).

The PATH variable looks like:
/Users/henning/.sdkman/candidates/mvnd/current/bin:/Users/henning/.sdkman/candidates/maven/current/bin:...

Since mvnd 0.9.0 the bin-directory contains a 'mvn', which is the first one the shell found. So the 'mvn' from the maven bin-directory is not used.

Is this an issue for mvnd or sdkman? I feel like ordering should not matter for PATH variables.

@marc0der
Copy link

marc0der commented Feb 8, 2023

@gnodet We are receiving complaints from our SDKMAN users that the mvn command is being clobbered by mvnd. Is there any chance that this change can be reverted?

@y-higuchi
Copy link

Similar issue happening on homebrew side as well.
Not a PATH conflict but symlink problem.
Homebrew installed 'maven' formula creates symlink for mvn and mvnd tries to overwrite it and fail.

@gnodet
Copy link
Contributor

gnodet commented Feb 10, 2023

That's a good question. I wonder if mvnd should become just another maven installation, rather than a separate project (from a hombrew / sdkman point of view). So that people can switch between stock mvn or mvnd, just like they do between graalvm / temurin or another JDK.
Would that make sense ?

@jglick
Copy link
Contributor

jglick commented Feb 10, 2023

As an sdk user, I for one prefer the previous (0.8.2) situation: maven package provides the mvn binary, and mvnd package provides the mvnd binary. I wish for both to be the latest version promoted by their respective publishers (which might be different versions of the Maven library, as they are as of this writing). If I want to use mvnd, I will run mvnd. If I want to use stock Maven, I will run mvn. They are interchangeable in many, but not all, situations, and I certainly do not want to be flipping some global configuration back and forth between shell commands.

@gnodet
Copy link
Contributor

gnodet commented Feb 10, 2023

As an sdk user, I for one prefer the previous (0.8.2) situation: maven package provides the mvn binary, and mvnd package provides the mvnd binary. I wish for both to be the latest version promoted by their respective publishers (which might be different versions of the Maven library, as they are as of this writing). If I want to use mvnd, I will run mvnd. If I want to use stock Maven, I will run mvn. They are interchangeable in many, but not all, situations, and I certainly do not want to be flipping some global configuration back and forth between shell commands.

I agree that mvnd and mvn do behave slightly differently in some cases, however, the mvn script that now comes with mvnd >= 0.9.0 does not use the daemon and it should simply boot maven, so there should be no differences really. Have you hit any problem ? (other than using the maven 3.8.7 from mvnd 0.9.0 distribution instead of maybe another version that you did select with sdkman)

@marc0der
Copy link

@gnodet the problem isn't really with the behaviour of your script, but rather with the fact that your mvn script clobbers the real mvn provided by Maven. There can only be one mvn on the path.

@gnodet
Copy link
Contributor

gnodet commented Feb 10, 2023

@gnodet the problem isn't really with the behaviour of your script, but rather with the fact that your mvn script clobbers the real mvn provided by Maven. There can only be one mvn on the path.

Yes, I understood that. That's why I think mvnd may be better advertised as a maven distribution instead of being advertised as a separate project. Kinda like graalvm is considered a java flavour and not a separate project.
One of the main reason is that mvnd does not reuse an existing installed maven, it embeds it completely and currently has a release cycle following maven releases (albeit using a different versioning scheme, kinda like graalvm with its support for jdk 11 and jdk17).

@mthmulders
Copy link

That's why I think mvnd may be better advertised as a maven distribution instead of being advertised as a separate project.

While this is an approach that may work with SDKMAN! (but @marc0der should know much better than I do), I am not sure other package managers (such as Homebrew) support a similar approach.

@y-higuchi
Copy link

Not a homebrew expert, but mvnd formula probably need to declare that it conflicts with maven formula or declare it as keg_only so that it does not try to symlink (=don't try to show up in PATH).
https://docs.brew.sh/Formula-Cookbook#specifying-conflicts-with-other-formulae

What's the benefit of mvnd taking over mvn as a command?
Suggesting people to define an alias to those who want to use mvnd as drop in replacement of mvn not an option?

@famod
Copy link
Contributor

famod commented Feb 16, 2023

FWIW, I was hit by #795 because of this overlap/overwrite.
Removed all mvnd sdkman packages for now (which oddly leaves behind non-existing /home/fmo/.sdkman/candidates/mvnd/current/bin in PATH).

@marc0der
Copy link

@famod you will need to open a new terminal to clean up the residual state. Let us know if you still see this entry in your path after doing that.

@marc0der
Copy link

@gnodet any update on this yet? I don't think we can advertise mvnd as a Maven distribution because it isn't multiplatform like mvnd. Because this is still causing issues for our users, I'd prefer if we remove 0.9.0 from SDKMAN until this is resolved.

@famod
Copy link
Contributor

famod commented Feb 16, 2023

@famod you will need to open a new terminal to clean up the residual state. Let us know if you still see this entry in your path after doing that.

Ah, thanks @marc0der. I was merely opening a new tab in Ubuntu console, which obviously didn't suffice.

@marc0der
Copy link

Ah, thanks. I was merely opening a new tab in Ubuntu console, which obviously didn't suffice.

@famod It might be that you are using a login shell, which means that this problem might persist until you log out and back in again.

@famod
Copy link
Contributor

famod commented Feb 16, 2023

Ah, thanks. I was merely opening a new tab in Ubuntu console, which obviously didn't suffice.

@famod It might be that you are using a login shell, which means that this problem might persist until you log out and back in again.

@marc0der all is well with the PATH cleansing; just opening a new tab is not enough but opening an entirely new Terminal window works. I don't have to log out/in.

PS: Sorry for the offtopic spam.

@cstamas
Copy link
Member

cstamas commented Feb 16, 2023

I personally solved this (hacked?) by removing exec bits from ~/.sdkman/candidates/mvnd/0.9.0/bin/mvn

@gnodet
Copy link
Contributor

gnodet commented Feb 16, 2023

@marc0der I'm fine with removing 0.9.0 for now or any other quick fix. Not sure how to do that though.

Though I don't understand how the fact that mvnd is multiplatform is relevant. It also works on platforms where the native binary is not available, so this should not be a problem.

@marc0der
Copy link

I'm fine with removing 0.9.0 for now or any other quick fix. Not sure how to do that though.

Great, you can remove any version using our API, it has a DELETE method. If you struggle I can help. Also, you would need to make 0.8.9 the new candidate default.

Though I don't understand how the fact that mvnd is multiplatform is relevant.

This is very relevant as SDKMAN classifies this at a candidate level. Besides the fact that it simply isn't possible, I also don't feel comfortable adding mvnd to the Maven candidate.

@gnodet
Copy link
Contributor

gnodet commented Feb 16, 2023

I'm fine with removing 0.9.0 for now or any other quick fix. Not sure how to do that though.

Great, you can remove any version using our API, it has a DELETE method. If you struggle I can help. Also, you would need to make 0.8.9 the new candidate default.

Done

Though I don't understand how the fact that mvnd is multiplatform is relevant.

This is very relevant as SDKMAN classifies this at a candidate level. Besides the fact that it simply isn't possible, I also don't feel comfortable adding mvnd to the Maven candidate.

I've been testing that locally (by using sdk install maven 0.9.0-mvnd-39 /path/to/mvnd-0.9.0 and similar other calls, and this seems to work fine enough for me. I'll experiment a bit more. I'm actually working on modifying mvnd to support maven 3.9.x and 4.0.x with two different distributions generated from the same source tree (i.e. a single maven build / release would produce 2 distributions for a given platform, one embedding maven 3.9.x and another one embedding 4.0.x).

For the platform, I think it would make sense to publish a UNIVERSAL distribution containing just the mvnd.sh script as indicated by https://sdkman.io/vendors#endpoints in addition to the distributions containing the native mvnd binary.

@marc0der
Copy link

marc0der commented Feb 18, 2023

@gnodet I'd prefer it if we do not follow this approach on SDKMAN. Having multiple packages for mvnd will be confusing for our users.

I'd like us to keep things as they are (one native package per version per platform). I also do not want the mvnd package to clobber the Maven binary.

@gnodet
Copy link
Contributor

gnodet commented Mar 9, 2023

@gnodet I'd prefer it if we do not follow this approach on SDKMAN. Having multiple packages for mvnd will be confusing for our users.

I'd like us to keep things as they are (one native package per version per platform). I also do not want the mvnd package to clobber the Maven binary.

@marc0der So the next mvnd release will support both maven 3.9.x and 4.0.x, but it will provide 2 different distributions for each platform (not a single distribution supporting two different maven versions). The reason is that most of the code is common, so it's easier to maintain two distributions from a single source tree, rather than having to port any change to 2 different branches. See https://github.com/apache/maven-mvnd/actions/runs/4374385157.
I suppose those will have to be published as mvnd version = 1.0.0-m5-m39 and 1.0.0-m5-m40

Was your objection was related the above point ? Else is there another way that mvnd publish a distribution for non natively supported platforms ? Reading the doc, I would think publishing a UNIVERSAL flavour would work, but you'll know better...

On the mvnd / mvn binaries mix, I'll revert the change that mixes up mvnd and mvn binaries, though I may come back to it in a few months when things have matured a bit more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants