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

Ensure GPG temp dir can be created for long build folder paths on linux riscv64 #3663

Merged
merged 5 commits into from
Feb 21, 2024

Conversation

andrew-m-leonard
Copy link
Contributor

@andrew-m-leonard andrew-m-leonard commented Feb 20, 2024

Fixes #3659

linux riscv64 GPG has folder path length limitations, similar to Alpine.

Successful GPG creation test: https://ci.adoptium.net/job/build-scripts/job/jobs/job/evaluation/job/jobs/job/jdk21u/job/jdk21u-evaluation-linux-riscv64-temurin/142/

@andrew-m-leonard andrew-m-leonard self-assigned this Feb 20, 2024
@github-actions github-actions bot added the x-linux Issues that affect or relate to the x64/x32 LINUX OS label Feb 20, 2024
@andrew-m-leonard andrew-m-leonard requested a review from sxa February 20, 2024 16:05
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.

Is there a reason this needs to be an elif instead of adding it the section that detects Alpine? This feels like unnecessary duplication

sbin/prepareWorkspace.sh Outdated Show resolved Hide resolved
@andrew-m-leonard
Copy link
Contributor Author

Is there a reason this needs to be an elif instead of adding it the section that detects Alpine? This feels like unnecessary duplication

Not sure this complex if works in bash would it? hence I took the less elegant but works approach

if [[ -r /etc/alpine-release ] || [[ "${BUILD_CONFIG[OS_KERNEL_NAME]}" == "linux" ] && [ "${BUILD_CONFIG[OS_ARCHITECTURE]}" == "riscv64" ]] ] && [ "$(pwd | wc -c)" -gt 83 ]; then

@github-actions github-actions bot added the testing Issues that enhance or fix our test suites label Feb 20, 2024
@sxa
Copy link
Member

sxa commented Feb 21, 2024

Is there a reason this needs to be an elif instead of adding it the section that detects Alpine? This feels like unnecessary duplication

Not sure this complex if works in bash would it? hence I took the less elegant but works approach

if [[ -r /etc/alpine-release ] || [[ "${BUILD_CONFIG[OS_KERNEL_NAME]}" == "linux" ] && [ "${BUILD_CONFIG[OS_ARCHITECTURE]}" == "riscv64" ]] ] && [ "$(pwd | wc -c)" -gt 83 ]; then

This is the syntax you're looking for if we're aiming for always on alpine, but also when the path is >83 characters on Linux/riscv64:

if [ -r /etc/alpine-release ] || ( [ "${BUILD_CONFIG[OS_KERNEL_NAME]}" == "linux" ] && [ "${BUILD_CONFIG[OS_ARCHITECTURE]}" == "riscv64" ] && [ "$(pwd | wc -c)" -gt 83 ] ); then ...

@andrew-m-leonard
Copy link
Contributor Author

Is there a reason this needs to be an elif instead of adding it the section that detects Alpine? This feels like unnecessary duplication

Not sure this complex if works in bash would it? hence I took the less elegant but works approach

if [[ -r /etc/alpine-release ] || [[ "${BUILD_CONFIG[OS_KERNEL_NAME]}" == "linux" ] && [ "${BUILD_CONFIG[OS_ARCHITECTURE]}" == "riscv64" ]] ] && [ "$(pwd | wc -c)" -gt 83 ]; then

This is the syntax you're looking for if we're aiming for always on alpine, but also when the path is >83 characters on Linux/riscv64:

if [ -r /etc/alpine-release ] || ( [ "${BUILD_CONFIG[OS_KERNEL_NAME]}" == "linux" ] && [ "${BUILD_CONFIG[OS_ARCHITECTURE]}" == "riscv64" ] && [ "$(pwd | wc -c)" -gt 83 ] ); then ...

Thanks @sxa updated

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.

We could abstract out the length count from the expression, but sine the linter is happy I'm ok with this and the logic looks sound :-)

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.

LGTM

@sxa sxa merged commit 2ac962f into adoptium:master Feb 21, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Issues that enhance or fix our test suites x-linux Issues that affect or relate to the x64/x32 LINUX OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GPG path lengths too long on RISC-V evaluation builds
4 participants