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

remove set -eo pipefail #5296

Merged
merged 1 commit into from
May 8, 2024
Merged

remove set -eo pipefail #5296

merged 1 commit into from
May 8, 2024

Conversation

llxia
Copy link
Contributor

@llxia llxia commented May 8, 2024

This does not work on some machines.

fixes: #5295

Signed-off-by: Lan Xia <Lan_Xia@ca.ibm.com>
@llxia
Copy link
Contributor Author

llxia commented May 8, 2024

reran on the failed machine: https://ci.adoptium.net/view/Test_grinder/job/Grinder/9891/

@llxia llxia marked this pull request as ready for review May 8, 2024 21:35
Copy link
Contributor

@LongyuZhang LongyuZhang left a comment

Choose a reason for hiding this comment

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

LGTM

@LongyuZhang LongyuZhang merged commit 83bec27 into adoptium:master May 8, 2024
1 check passed
@judovana
Copy link
Contributor

judovana commented May 9, 2024

set -eo pipefail is quite crucial part of the script. Its removal may be breakig other platforms.

In addition, this PR is wrong. The message clearly says "set: Illegal option -o pipefail" so this PR should have remove only -o pipefail, and keep the e on.

Seeing also the #!/usr/bin/env bash in the script, is is supposed to be bash, bash should know -o pipefail unless it is ancient bash (or fake bash).

@llxia - please update your systems. You have to be run exceptionally legacy configurations, and it is enforcing breaking changes for everybody else.

In addition, the https://github.com/llxia/openjdk-tests/blob/cb86453541d2cedb78dab55e0dac63905c14ad6d/scripts/testenv/testenvSettings.sh do not have isngle pipe so

- set -eo pipefail
+ set -e

would be very correct fix. But

- set -eo pipefail

Thus removing whole -e is usualy very bad fix. Please revert if you can.

@llxia
Copy link
Contributor Author

llxia commented May 9, 2024

please update your systems.

Thanks @judovana . Please feel free to open an Adoptium infra issue for updating the system.
For now, we need to get the changes in to unblock the regular builds. We will add some checks in the separate PR.

@judovana
Copy link
Contributor

judovana commented May 9, 2024

Then at least return the -e

llxia added a commit to llxia/aqa-tests that referenced this pull request May 9, 2024
Signed-off-by: Lan Xia <Lan_Xia@ca.ibm.com>
pshipton pushed a commit that referenced this pull request May 9, 2024
Signed-off-by: Lan Xia <Lan_Xia@ca.ibm.com>
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

Successfully merging this pull request may close these issues.

Error message set: Illegal option -o pipefail
4 participants