Skip to content
This repository has been archived by the owner on Dec 18, 2021. It is now read-only.

Use of bashisms make this script fail in minimal /bin/sh implementations like dash #34

Closed
jvz opened this issue Feb 13, 2017 · 16 comments
Closed

Comments

@jvz
Copy link

jvz commented Feb 13, 2017

In mvnw, the use of $(pwd) to invoke commands instead of `pwd` to do it is a bashism. This can be fixed either by using the sh-compatible backtick version or specifying /bin/bash in the shebang. Some GNU/Linux distributions ship with minimal sh-compatible shells as the default /bin/sh instead of bash. Debian switched to dash a while back, and I think Ubuntu followed that as well but I haven't checked.

@ppalaga
Copy link

ppalaga commented Mar 7, 2017

Yes, mvnw does not work also on Solaris 10 and 11. Once my fix proposal [1] passes our QA, I'll port it here.

[1] wildfly/wildfly@aa86cf1#diff-7f6d424362f5b03d9b91a80a1e43eefe

@jvz
Copy link
Author

jvz commented Mar 7, 2017

I've never seen a subexpression within backticks like that before. Interesting!

@mosabua
Copy link
Member

mosabua commented Apr 10, 2017

The current version of the script no longer has those bashisms and I just test it on dash and it works. I will release 0.2.0 and a new maven plugin in the next days to fix this with a release.

@mosabua mosabua closed this as completed Apr 10, 2017
@ppalaga
Copy link

ppalaga commented Apr 10, 2017

@mosabua there is still a few occurrences of $(...) in the current mvnw due to which it will fail on Solaris 10 and 11. We are in the second round of testing of our fixes and the chances are good that our solution will pass QA on Solaris 10 and 11 plus all other platforms JBoss EAP is tested on. This is what we have ATM: https://github.com/wildfly/wildfly/blob/4dbebcc9c64ba6b1e2303b8049ba153fb2b5ef01/mvnw

My promise to port our fixes here once they pass our QA is still valid, but QA still needs a few days to complete.

Please re-open this issue or I will file a new one that will sound very similar to the present one.

@ppalaga
Copy link

ppalaga commented Apr 10, 2017

cc @rpelisse, @rsvoboda, @marekkopecky

@mosabua mosabua reopened this Apr 10, 2017
@mosabua
Copy link
Member

mosabua commented Apr 10, 2017

I tested the current script in master with dash and it works fine. Let me know how it goes and if you provide a PR please make sure it is based on the current master.

@ppalaga
Copy link

ppalaga commented Apr 10, 2017

@mosabua thanks for re-opening

@mosabua
Copy link
Member

mosabua commented Apr 10, 2017

I am hoping to cut a release in the next days.. and from my testing things look good in bash, dash and zsh .. all on Ubuntu however... I also tested osx and windows.

@ppalaga
Copy link

ppalaga commented Apr 11, 2017

Our experience says that Solaris is very special.

@marekkopecky would you please try to find a way to verify that the changes in [1] work on all platforms supported by us, so that @mosabua can release maven-wrapper ASAP?

[1] https://github.com/wildfly/wildfly/blob/4dbebcc9c64ba6b1e2303b8049ba153fb2b5ef01/mvnw

@rpelisse
Copy link

rpelisse commented Apr 11, 2017

IF we have more issue with Solaris 10 in the future, we will need to change our strategy. We can't just - pardon my french[1], fucked up more and more the syntax of all the bash scripts, just to please a bogus, not compliant interpreter.

As far as I've seen Solaris 11 seems to be working fine with substitution and other "bashisms" - so if we run into more trouble with Solaris 10, we should try to update the interpreter on the system, rather than hacking the script (IMHO)

@jvz if it feels my heart with joy that you have never backtick before - at least practises overall are going in the right direction...

[1] I get a pass on this one, because I'm actually french, so ...

@marekkopecky
Copy link

@ppalaga I'm busy this week, but @rpelisse can test it too, he has access to Solaris machines.

@rpelisse
Copy link

@marekkopecky I already did validate that the change (already merged in wildfly btw) works.

@mosabua as far as I know you are good to go

@ppalaga
Copy link

ppalaga commented Apr 11, 2017

@rpelisse the changes from https://github.com/wildfly/wildfly/blob/4dbebcc9c64ba6b1e2303b8049ba153fb2b5ef01/mvnw still need to ported here. @mosabua please hold on, I'll port them today or tomorrow.

@mosabua
Copy link
Member

mosabua commented Apr 11, 2017

@ppalaga the script works quite a bit different now so you might want to verify that the current version works .. I tested it on dash on Ubuntu and its fine. If you port anything it will need to port to the current master.. and dont worry - I am waiting for your contrib.

@rpelisse
Copy link

@mosabua I've did the PR for @ppalaga - here it is: PR39. Let me know if you see any issue with it.

@mosabua mosabua closed this as completed Apr 17, 2017
@mosabua
Copy link
Member

mosabua commented Apr 17, 2017

Fyi maven-wrapper:0.2.0 and and takari-maven-plugin:0.4.0 are released to Central.

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

No branches or pull requests

5 participants