Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update parsing TEMURIN_BUILD_ARGS #3824
Update parsing TEMURIN_BUILD_ARGS #3824
Changes from all commits
5ebfb93
d98d20e
55ce82b
1415629
8af8e39
75e969a
b2050b5
6919b17
5ebad2c
636999a
0ee465d
2ccf570
c614969
baafa00
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally go with this as it feels less complex than using an array - you can add extra things to filter out later with multiple
-e
parameters if needed. But I'm ok with either approach. We could also combine this with the jdk-boot-dirsed
below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a code standard issue. I personally feel the array is simpler if extra options need to be filtered out. Using multiple -e make it less readable and more error-prone for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this logic is working. When I tried it out it converted:
in the string to:
so if you want to leave it with the bash-specific construct, I think you'll need to switch
//
to/
in the translation expression.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused. @sxa which logic did you try? Seems you were trying
buildArgs="$(echo buildArgs | sed -e 's/--enable-sbom-strace//g')"
? As I said I would prefer using the array, similar to the window reproducible comparing script.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the version with sed works ok - the current version with the array adds in an extra
/
unless you remove one of them as you can see in this example:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that this may be a little confusing to anyone reading this to see a local variable
using_DEVKIT
and a global oneUSING_DEVKIT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra blank line here should be removed