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

Fix error message when package install fails due to missing Java #36077

Merged
merged 4 commits into from Dec 3, 2018

Conversation

Projects
None yet
5 participants
@dliappis
Copy link
Contributor

commented Nov 29, 2018

Currently if java is not in $PATH the preinst script fails
prematurely and prevents an appropriate message from getting displayed
to the user.

Make package installation more user friendly when java is not in
$PATH, by emitting a message to stderr. Also improve the current test case for
installation exit 1 when java isn't present by also testing for the output of
the error message.

Finally use a she-bang in the preinst script, as, at least in Debian,
maintainer scripts must start with the #! convention [1].

Relates: #31845

[1] https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html

dliappis added some commits Nov 27, 2018

Fix error message when package install fails due to missing Java
Currently is `java` is not in $PATH the preinst script fails
prematurely and prevents an appropriate message from getting displayed
for the user.

Make package installation more user friendly when java is not in
$PATH.

Also use a she-bang in the preinst script, as, at least in Debian,
maintainer scripts must start with the #! convention [1].

Relates: #31845

[1] https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Nov 29, 2018

@jasontedor
Copy link
Member

left a comment

Looks good, I left one comment for change and one question.

@@ -45,7 +45,7 @@ else
fi

if [ ! -x "$JAVA" ]; then
echo "could not find java; set JAVA_HOME or ensure java is in PATH"
echo "could not find java; set JAVA_HOME or ensure java is in PATH" >&2

This comment has been minimized.

Copy link
@jasontedor

jasontedor Nov 29, 2018

Member

I notice that you changed the mode on this file. Is that intentional? We do not intend for this file to be executed directly, so it doesn't need the execute bit set.

This comment has been minimized.

Copy link
@dliappis

dliappis Nov 30, 2018

Author Contributor

I did? I only redirected the err msg to stderr instead of stdout, am I missing something?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Nov 30, 2018

Member

Yes, the change in permissions shows up in the diff 100644 → 100755. I can't comment anywhere else except on this line in the file yet I'm talking about the file permissions on distribution/src/bin/elasticsearch-env.

This comment has been minimized.

Copy link
@dliappis

dliappis Nov 30, 2018

Author Contributor

Thanks, good catch! Truly odd, I definitely didn't do any chmod's yet something changed it. I've reverted it in 1b4f895

@@ -1,3 +1,4 @@
#!/bin/sh

This comment has been minimized.

Copy link
@jasontedor

jasontedor Nov 29, 2018

Member

This should be /bin/bash, that's what all of our scripts require.

This comment has been minimized.

Copy link
@andyb-elastic

andyb-elastic Nov 29, 2018

Contributor

++ I think consistency on this is especially important for people trying to figure out what we assume prior to installation

This comment has been minimized.

Copy link
@dliappis

dliappis Nov 30, 2018

Author Contributor

If we could switch to /bin/bash it would be great to make the code more readable too e.g. conditionals etc. as now I was limited to POSIX only. Defaulting to /bin/bash however, I fear it is dangerous, as the script is common across all platforms and there's no guarantee that /bin/bash is present on all platforms, e.g. Debian defaults to /bin/dash.

This comment has been minimized.

Copy link
@dliappis

dliappis Nov 30, 2018

Author Contributor

However, our packages already depend on bash so we should be safe. I will commit this change ASAP. Thanks!

This comment has been minimized.

Copy link
@dliappis

dliappis Nov 30, 2018

Author Contributor

Addressed in f1fd2a2

This comment has been minimized.

Copy link
@jasontedor

jasontedor Nov 30, 2018

Member

We require /bin/bash since a long time. 😇

@andyb-elastic
Copy link
Contributor

left a comment

Looks good.

When I added PackageTestCase, I tried to do something similar (echoing that java message to stderr instead of stdout) and found it didn't work in one type of package (can't remember if it was deb or rpm) because the message just was never outputted. But it looks like the full packaging tests are passing here so I must have been doing something wrong. So nothing actionable but just wanted to note that

@dliappis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

When I added PackageTestCase, I tried to do something similar (echoing that java message to stderr instead of stdout) and found it didn't work in one type of package (can't remember if it was deb or rpm) because the message just was never outputted.

Thanks @andyb-elastic ! The reason the err message wasn't outputted was that which $JAVA was causing the script error prematurely (when java wasn't found :) ). The bad coincidence was that which also exited with the same err code, so rather difficult to track.

dliappis added some commits Nov 30, 2018

@jasontedor
Copy link
Member

left a comment

LGTM.

@dliappis dliappis removed the request for review from nik9000 Nov 30, 2018

@dliappis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

@jasontedor Any suggestions how far back to go wrt backports?

@jasontedor

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

@dliappis On top of master, 6.5 and 6.x.

@dliappis dliappis merged commit 6a773d7 into elastic:master Dec 3, 2018

7 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci-1 Build finished.
Details
elasticsearch-ci-2 Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

dliappis added a commit that referenced this pull request Dec 3, 2018

Fix error message when package install fails due to missing Java (#36077
)

Currently is `java` is not in $PATH the preinst script fails
prematurely and prevents an appropriate message from getting displayed
to the user.

Make package installation more user friendly when java is not in
$PATH and add a test for it.

Also use a she-bang in the preinst script, as, at least in Debian,
maintainer scripts must start with the #! convention [1].

Relates #31845

[1] https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html

dliappis added a commit that referenced this pull request Dec 3, 2018

Fix error message when package install fails due to missing Java (#36077
)

Currently is `java` is not in $PATH the preinst script fails
prematurely and prevents an appropriate message from getting displayed
to the user.

Make package installation more user friendly when java is not in
$PATH and add a test for it.

Also use a she-bang in the preinst script, as, at least in Debian,
maintainer scripts must start with the #! convention [1].

Relates #31845

[1] https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html
@dliappis

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

Backported:

6.x -> 790238b
6.5 -> 7e40b47

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