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
enabled build from source dir/tarball via to --openjdk-source-location/-l #3737
enabled build from source dir/tarball via to --openjdk-source-location/-l #3737
Changes from 35 commits
a635458
b375a2a
70278b4
bf63854
d7a1ef3
433ea0f
0620e50
8cb6d80
e0ccb12
6c1bafb
7030bcd
eabc2c2
5aa6407
8c2753c
d1de822
8f8bc10
176ee8a
742b05a
38e318f
c8c32da
fd8f48e
6b71133
e954eb5
098aacd
d661188
4c06221
d84c039
b459e69
194aabb
6b342d2
9ed221a
57dda8a
519c528
c3cc00f
79bf173
43b5c93
45e4efb
23454cc
5e857d3
d8304b9
faf2bea
72d3ed9
a399523
22299e6
9140925
7edc874
5dcb140
385bfc3
793fcad
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.
Archive to me usually means .zip or .tar etc. Is OPENJDK_FOREST_SOURCE or something OPENJDK_LOCAL_SOURCE a better name?
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 can be archive archive. You can build local directory and tarball (both with sources) in same way.
As i replied to @sxa #3737 (comment) the proepr name aka
OPENJDK_LOCAL_SOURCE_DIR_OR_ARCHIVE
is terrible, but why not.and
both do same job.
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.
actually,
OPENJDK_LOCAL_SOURCE
andOPENJDK_LOCAL_SOURCE_ABSPATH
are generic enough. @sxa , I will use that.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.
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.
Hello! Not following this one unless you insists. If you look carefully the block in pushd/popd is indented:
I don't know how global this habit is, but its usefulness proved over years
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.
Not seen that before, but I'm ok with it :-)
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.
The indentation or popd itself? The indentation of insides of pushd/popd block is quite good habbit.
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 feels a little odd to be calling this from within
checkoutAndCloneOpenJDKGitRepo
- perhaps we could invoke it from the higher level configureWorkspace` instead as an alternative. I guess it depends how much of the extra logic in this function we'd need to duplicate or abstract out.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.
ok. let me try.
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.
Oh I see. There is
So the change of control dir, which is pretty important for any future steps would need to be handled.
And the main if is still using the
[ "${BUILD_CONFIG[OPENJDK_LOCAL_SOURCE_ARCHIVE]}" == "false" ]
becasue of the change of the dir, I would rather not chnage the location of the call, unless yo really insists.
In that case it will go :
where the wrapper:
IN addition the method of
checkoutAndCloneOpenJDKGitRepo
is doing a bit more beforecd "${BUILD_CONFIG[WORKSPACE_DIR]}"
Although it seems those lines looks oook to omit (temurin exit, update openj9 and createSourceTagFile ) I'm not sure If I feel safe to omit themThere 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.
Yeah agree that the safest option is to leave it as-is although putting it in the
configureWorkspace
function as per your example does look a lot nicer. But I won't insist on it :-)