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

Use standard sed syntax for translating filenames #4072

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

sxa
Copy link
Member

@sxa sxa commented Dec 2, 2024

Solaris's sed doesn't support the -r parameter. Since I believe this was only required for the OR section on the extension I've split it out into two separeate -e parameters for sed. One for the .tar.gz case and the other for .zip. I've also escaped the brackets.

To test:

X=jdk8u-hotspot.tar.gz
echo $X | sed -e "s/\(.*\)\(\.tar\.gz\)/\1-$component\2/" -e "s/\(.*\)\(\.zip\)/\1-${component}\2/"

Noting that even on Linux the original version was likely not working:

X=jdk8u-hotspot.tar.gz
echo $X | sed -e "s/(.*)(\.tar.gz)/\1-component\2/"   
sed: -e expression #1, char 32: invalid reference \2 on `s' command's RHS
echo $X | sed -e "s/\(.*\)\(\.tar.gz\)/\1-component\2/"   
jdk8u-hotspot-component.tar.gz

Testing at https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk8u/job/jdk8u-solaris-x64-temurin-simple/17/

Identified as part of adoptium/infrastructure#3742

@sxa sxa requested a review from netomi December 2, 2024 19:28
@sxa sxa self-assigned this Dec 2, 2024
@github-actions github-actions bot added solaris Issues that affect or relate to the SOLARIS OS testing Issues that enhance or fix our test suites labels Dec 2, 2024
Copy link
Contributor

@netomi netomi left a comment

Choose a reason for hiding this comment

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

The mentioned reasoning is correct, the '|' operator is not available in basic regexp only extended. The workaround works well imho after doing a test.

@@ -2094,7 +2094,9 @@ getTargetFileNameForComponent() {
echo "${target_file_name}" | sed "s/-jdk/-${component}/"
else
# if no pattern is found, append the component name right before the extension.
echo "${target_file_name}" | sed -r "s/(.+)(\.tar\.gz|\.zip)/\1-${component}\2/"
echo "${target_file_name}" | sed \
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth adding a comment about using Solaris friendly sed? I've seen a few cases where this sort of thing gets reverted by Linux focused folks :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't shell check able to to check posix? But I guess that is syntax only and not sed/grep and others...

Copy link
Member Author

@sxa sxa Dec 3, 2024

Choose a reason for hiding this comment

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

Worth adding a comment about using Solaris friendly sed? I've seen a few cases where this sort of thing gets reverted by Linux focused folks :-)

I have a view on that ... If we think we really need to tell people explictly "Don't change this to a non-standard syntax" I can add it (such things aren't just for Solaris)... however I feel adding such comments everywhere would be a job for life ;-) "Linux folks" need to be aware that this isn't just a Linux project when copying stuff from stackoverflow/ChatGPT etc. ;-)

Isn't shell check able to to check posix? But I guess that is syntax only and not sed/grep and others...

I suspect so yeah. There have also been cases where we have used if blocks to explicitly check for and use non-standard versions, so they'd have to be skipped.

sxa and others added 2 commits December 3, 2024 10:39
@sxa sxa requested a review from steelhead31 December 3, 2024 11:10
Copy link
Contributor

@steelhead31 steelhead31 left a comment

Choose a reason for hiding this comment

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

Looks good :)

@sxa sxa merged commit aab0db6 into adoptium:master Dec 3, 2024
26 checks passed
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 testing Issues that enhance or fix our test suites
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants