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 joinPathOS method that fails on solaris #3572

Merged
merged 6 commits into from
Dec 14, 2023

Conversation

netomi
Copy link
Contributor

@netomi netomi commented Dec 11, 2023

Previously printf was used which behaves differently on solaris it seems.

Using a method that should work anywhere.

Use IFS='/' to separate path elements and remove duplicate slashes afterwards using tr.

Fixes #3571

…r to remove duplicate slashes which should work anywhere.
@github-actions github-actions bot added the solaris Issues that affect or relate to the SOLARIS OS label Dec 11, 2023
@github-actions github-actions bot added solaris Issues that affect or relate to the SOLARIS OS and removed solaris Issues that affect or relate to the SOLARIS OS labels Dec 11, 2023
Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

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

Looks OK but could do with a comment saying why we are using IFS

@github-actions github-actions bot added solaris Issues that affect or relate to the SOLARIS OS and removed solaris Issues that affect or relate to the SOLARIS OS labels Dec 12, 2023
@netomi
Copy link
Contributor Author

netomi commented Dec 12, 2023

Looks OK but could do with a comment saying why we are using IFS

added some comment

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

I'm a bit unclear as to why we need IFS in this case - is the outcome of the joinPath any different from the somewhat simpler:

echo "${@}" | tr ' ' /

Or are there some edge cases that would be missed with that? I'm probably missing something obvious but thought it was worth checking :-)
(EDIT: I've missed out the tr -s on there from your change that's likely desirable too)

@netomi
Copy link
Contributor Author

netomi commented Dec 12, 2023

I think that would also work:

echo "/${@}" | tr ' ' / | tr -s /

I wanted to have a reliable way to construct a valid path from components without having to worry about slashes and remove duplicate ones. While this is not necessary according to the POSIX standard, it still looks weird in logs.

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

I think that would also work:
echo "/${@}" | tr ' ' / | tr -s /
I wanted to have a reliable way to construct a valid path from components without having to worry about slashes and remove duplicate ones. While this is not necessary according to the POSIX standard, it still looks weird in logs.

Yeah most things will work without the duplicate removal, but I agree that it's nicer to do it. I'd personally definitely prefer the two tr operations over the use of IFS as (to me at least) it's a lot clearer. But I'll approve regardless of which option you go for as the most important thing is to avoid the build break.

@github-actions github-actions bot added solaris Issues that affect or relate to the SOLARIS OS and removed solaris Issues that affect or relate to the SOLARIS OS labels Dec 13, 2023
@github-actions github-actions bot added solaris Issues that affect or relate to the SOLARIS OS and removed solaris Issues that affect or relate to the SOLARIS OS labels Dec 13, 2023
@netomi
Copy link
Contributor Author

netomi commented Dec 13, 2023

applied the change and removed the comment again. The comment was requested to explain the use of IFS, I think right now its pretty straight forward.

@github-actions github-actions bot added solaris Issues that affect or relate to the SOLARIS OS and removed solaris Issues that affect or relate to the SOLARIS OS labels Dec 13, 2023
@sxa sxa merged commit 4d60aef into adoptium:master Dec 14, 2023
19 of 23 checks passed
@sxa
Copy link
Member

sxa commented Dec 14, 2023

@sxa
Copy link
Member

sxa commented Dec 14, 2023

LGTM - OpenJDK SBOM will be /export/home/jenkins/workspace/build-scripts/jobs/jdk8u/jdk8u-solaris-sparcv9-temurin/workspace/target/OpenJDK8U-sbom_sparcv9_solaris_hotspot_2023-12-14-09-42.json.

@karianna karianna mentioned this pull request Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solaris Issues that affect or relate to the SOLARIS OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jdk8u Solaris build break due to failing SBOM generation
3 participants