allowing build from diectory/src snapshot #3724
Conversation
judovana
changed the title
|
Note that I intentionally added, and left in some |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
A block has been put on this Pull Request as this repository is temporarily under a code freeze due to an ongoing release cycle.
If this pull request needs to be merged during the release cycle then please comment /merge and a PMC member will be able to remove the block.
If the code freeze is over you can remove this block by commenting /thaw.
|
@judovana I'm not understanding the use case here. The build scripts already support building from a local directory via the |
|
The usecase is to build local random dir/local jdk work in progress/local source snaphshot
Hmm. You understand the scripts hundred times more then me, so you are most likely right. Even If I'm wrong (which most likely I'm) the scripts expects git repo (and .git) folder to be present. So that is something I need to get rid to build random dir/source snapshot. That is moreover 100% of this work. I was plying with faking .git repo, in the copy of the random dir/unpacked source snapshot, but that was fragile, and was actually more nasty patch. Last thing, which is based on previous) the make_any_jdk is fiddling with the repo to much - various reserts, checkouts and so, which moreover prohibits building local copy where is work in progress. But yet again, I'm most likely very wrong and had misunderstood how it should work, or missed set of switches allowing me to build local random dir/local jdk work in progress/local source snaphshot ps, I had notyiced the shell check issues, most of them are valid, will elaborate as thsi review will progress. |
|
The On current master: thus with this change, it behaved correctly: Thanx for pointer! I have most liekly missed much more cases like this... Actually this exercise showed one more intereresting point. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@judovana i'm afraid I don't quite understand the "use case", can you explain this statement from your issue please?
Maybe provide a few examples? |
|
please any admin, unhide #3724 (comment) ; I do nto have permissions:( It was not supposed ot be hidden |
|
And I was hoping to be self describing enough :) The goal is simple - I have a source tarball or directory, and I want temurin-build to build it for me. No cloning, no playing with tags or branches, ok with no .git dir. From all I know current scripting do not offer this functionality, and I heavily miss it. Note that I'm perfectly familiar with jdk build process, and I can turn jdk code-build-test cycles on my own and pretty quickly. This is additional tier - full temurin-close build. this is providing the feature: |
this is partially bound to adoptium#3724, where dsah and undersocre are oneof major delimiters in directory names jdk11u-my_feature May, or may have not sense without the 3724
this is partially bound to adoptium#3724, where dash and underscore are one of major delimiters in directory names jdk11u-my_feature May, or may have not sense without the 3724
| @@ -18,6 +18,19 @@ | |||
| # shellcheck disable=SC2153 | |||
| function setOpenJdkVersion() { | |||
| local forest_name=$1 | |||
| set -x | |||
| if [ -e "${forest_name}" ] ; then | |||
There was a problem hiding this comment.
I don't think this is the right place for this logic.
- Existing build arg --version <forest_name> can be used instead of OPENJDK_FOREST_ALTID
- I think specify the openjdk srcDirectory/Tarball via a build arg of say : --openjdk-source <full path to srcDirectory/Tarball> and that sets the variable OPENJDK_FOREST_SOURCE
There was a problem hiding this comment.
There are two things mixed togehter.
Yyou are absolutly correct and --version is exactly what OPENJDK_FOREST_ALTID was substituing. I will remove --jdk-dir-altid in favour of --version It will need some more changes, nut better then additional single purposed switch. Sorry for missing --version.
I woudl very like to avoid the --openjdk-source. it would kill the script logic. Teh script expects repository_name or dir/tarball. The usage of make_any_jdk.sh dir is much more strigthforward and much usable then the verbose one. In additoon, the script will be pulluted by nasty (imo) if:
"expects exactly one argument, unless --openjdk-source si used, then it expects no argument"
Unless you are expecting the usage as make_any_jdk.sh --openjdk-source some_terrible_name/path/zip id`
Where id is still mandatory, and is providing the necessary "original repo" information.
I like "my" approach a bti more, because it have less mandatory arguments
make_any_jdk.sh some_terrible_name/path/zip+optionalversion id- because the name/path/zip maybe akready proper version.
In comaprison to
make_any_jdk.sh --openjdk-source some_terrible_name/path/zipwith mandatory --openjdk-source and otional short id.
wdyt?
There was a problem hiding this comment.
So maybe, the syntax of makejdk-any-platform.sh, is:
makejdk-any-platform.sh <jdk_version>
Where jdk_version is what has historically been named as the "forest version"
From jdk_version you must be able to set:
BUILD_CONFIG[OPENJDK_CORE_VERSION] eg. jdk21
BUILD_CONFIG[OPENJDK_FOREST_NAME] eg. jdk21u
BUILD_CONFIG[OPENJDK_FEATURE_NUMBER] eg. 21
These are needed throughtout the sbin/build.sh build logic.. So you will need to set these from some_terrible_name/path/zip ??
There was a problem hiding this comment.
I would actually prefer:
makejdk-any-platform.sh jdk21u --openjdk-source some_terrible_name/path/zip
because the existing logic very clearly set:
BUILD_CONFIG[OPENJDK_CORE_VERSION] eg. jdk21
BUILD_CONFIG[OPENJDK_FEATURE_NUMBER] eg. 21
There was a problem hiding this comment.
So maybe, the syntax of makejdk-any-platform.sh, is:
makejdk-any-platform.sh <jdk_version>Where jdk_version is what has historically been named as the "forest version" From jdk_version you must be able to set:
BUILD_CONFIG[OPENJDK_CORE_VERSION] eg. jdk21 BUILD_CONFIG[OPENJDK_FOREST_NAME] eg. jdk21u BUILD_CONFIG[OPENJDK_FEATURE_NUMBER] eg. 21These are needed throughtout the sbin/build.sh build logic.. So you will need to set these from some_terrible_name/path/zip ??
Exactly, And thats what that dir/zip i susually named. See #3727
For cases Where the the dir/tarball is nonsense from this pint of vie, I introduced that altid, which will be substituted by version. And used that to set featureversion, if used.
There was a problem hiding this comment.
I would actually prefer:
makejdk-any-platform.sh jdk21u --openjdk-source some_terrible_name/path/zipbecause the existing logic very clearly set:
BUILD_CONFIG[OPENJDK_CORE_VERSION] eg. jdk21 BUILD_CONFIG[OPENJDK_FEATURE_NUMBER] eg. 21
Probably my main reason is to save typing --openjdk-source every time I wish to build some properly named directory:( I was resuing the logic which set
BUILD_CONFIG[OPENJDK_CORE_VERSION] eg. jdk21 BUILD_CONFIG[OPENJDK_FEATURE_NUMBER] eg. 21
properly. Do you really insists?
| if [ -e "${forest_name}" ] ; then | ||
| BUILD_CONFIG[OPENJDK_FOREST_DIR]="true" | ||
| BUILD_CONFIG[OPENJDK_FOREST_DIR_ABSPATH]=$(readlink -f "${forest_name}") | ||
| BUILD_CONFIG[DISABLE_ADOPT_BRANCH_SAFETY]="true" |
There was a problem hiding this comment.
User should specify build arg: --disable-adopt-branch-safety
There was a problem hiding this comment.
It was breaking in the branching block. I can remove this, but conisder:
- the branch safety is usels above tarball/dir without .git
- I will need to add if into the block using
BUILD_CONFIG[DISABLE_ADOPT_BRANCH_SAFETY]
I will obey as you need, but the setup of it sounds to me more clean and less harmfull.
| # we want to use "." and similar | ||
| forest_name=$(basename "${BUILD_CONFIG[OPENJDK_FOREST_DIR_ABSPATH]}") | ||
| fi | ||
| BUILD_CONFIG[OPENJDK_SOURCE_DIR]="${forest_name}" |
There was a problem hiding this comment.
don't set OPENJDK_SOURCE_DIR, as that is set to the folder under the WORK_DIR, default "src", and is where the git clone of openjdk clones the source.
The unzipping/copying of the forest source, should unzip/copy to the OPENJDK_SOURCE_DIR
There was a problem hiding this comment.
Yah, I agree, I think I set it because I was unsure how to proeprly set the name to be read by. Will elaborate and fix
| @@ -82,7 +82,10 @@ NUM_PROCESSORS | |||
| OPENJDK_BUILD_NUMBER | |||
| OPENJDK_CORE_VERSION | |||
| OPENJDK_FEATURE_NUMBER | |||
| OPENJDK_FOREST_ALTID | |||
There was a problem hiding this comment.
I think all you need is a OPENJDK_FOREST_SOURCE build arg
There was a problem hiding this comment.
I think you mean --version ? Thats what I will use instead of OPENJDK_FOREST_ALTID. The directory/tarball should remain the only mandatory argument, which was name of the repo untill now.
| @@ -24,7 +24,7 @@ | |||
| ################################################################################ | |||
|
|
|||
| set -eu | |||
|
|
|||
| set -x | |||
There was a problem hiding this comment.
Sure thing. before merge, all -x will be removed. I think there is still quite some iterations ahead. of us. Thanx a lot fo reding it through!
| @@ -37,14 +37,52 @@ ALSA_LIB_CHECKSUM=${ALSA_LIB_CHECKSUM:-5f2cd274b272cae0d0d111e8a9e363f0878332915 | |||
| ALSA_LIB_GPGKEYID=${ALSA_LIB_GPGKEYID:-A6E59C91} | |||
| FREETYPE_FONT_SHARED_OBJECT_FILENAME="libfreetype.so*" | |||
|
|
|||
|
|
|||
| copyFromDir() { | |||
There was a problem hiding this comment.
copyFromDir() and unpackFromArchive() need to "copy"/"unpack" the forest openjdk source to where the build scripts intends to build the source within the defined WORKING_DIR,
ie. both these methods need to end up copying the source to be built to the location:
"${BUILD_CONFIG[WORKSPACE_DIR]:?}/${BUILD_CONFIG[WORKING_DIR]}/${BUILD_CONFIG[OPENJDK_SOURCE_DIR]}"
There was a problem hiding this comment.
The original clonnign script is using the "." too, if that is what you are reffereing. I was wondering too that there is no ful apth, but just "."
I will heapily switch to full path instead of "." but allt he cloning methods were really exppecting other parts to cd for them. Can you confirm please?
| echo "Not directory nor file ${BUILD_CONFIG[OPENJDK_FOREST_DIR_ABSPATH]}" | ||
| exit 1 | ||
| fi | ||
| rm -vf ./${BUILD_CONFIG[OPENJDK_FOREST_NAME]}/build/*/configure-support/config.status |
There was a problem hiding this comment.
this should not be needed as the build-scripts will clean the autoconf configuration in the intended build location.
There was a problem hiding this comment.
Unluckily it is necessary. The methods later in build.sh, are not expoecting prexsiting configuration, and the builds were failing.
There was a problem hiding this comment.
So you copyOrUntarZip() methods should be copying/untaring the source to where temurin-build scripts is doing the build, ie. to ${BUILD_CONFIG[WORKSPACE_DIR]}/${BUILD_CONFIG[WORKING_DIR]}/${BUILD_CONFIG[OPENJDK_SOURCE_DIR]}
So you can simply exclude copying "build/*"
There was a problem hiding this comment.
- Thats what they do (the copying is on correect place, to correct destination, simply substituting instea of the cloned repo.
- I will try to exclude build. Will be indeed better.
| # Create a new clone or update the existing clone of the OpenJDK source repo | ||
| # TODO refactor this for Single Responsibility Principle (SRP) | ||
| checkoutAndCloneOpenJDKGitRepo() { | ||
|
|
||
| cd "${BUILD_CONFIG[WORKSPACE_DIR]}/${BUILD_CONFIG[WORKING_DIR]}" | ||
|
|
||
| # Check that we have a git repo, we assume that it is a repo that contains openjdk source | ||
| if [ -d "${BUILD_CONFIG[OPENJDK_SOURCE_DIR]}/.git" ]; then | ||
| if [ -d "${BUILD_CONFIG[OPENJDK_SOURCE_DIR]}/.git" -a "${BUILD_CONFIG[OPENJDK_FOREST_DIR]}" == "false" ]; then |
There was a problem hiding this comment.
use && to be consistent with this script
There was a problem hiding this comment.
ouch. Sorry. Will do!
|
|
||
| if [ -d "${BUILD_CONFIG[OPENJDK_SOURCE_DIR]}/.git" ]; then | ||
| if [ "${BUILD_CONFIG[OPENJDK_FOREST_DIR]}" == "true" ]; then | ||
| echo "Version: local dir; OPENJDK_BUILD_NUMBER set as ${BUILD_CONFIG[OPENJDK_BUILD_NUMBER]}" |
There was a problem hiding this comment.
OPENJDK_BUILD_NUMBER is in fact just the openjdk build number, so for example jdk-21.0.2+13, the build number is just "13"
I think you probably want to use "TAG" in this echo statement, and when you build from your own forest source you would pass arguments:
--openjdk-source <path_to_tarball>
--tag jdk-17.0.12+4
There was a problem hiding this comment.
Nope. Surprisingly in this whole enhancement I was not calculating with the tag. It is jsut tarball/dir - I do pretend I do not care baout whatever version it is/was. However jdk major "the number" is absolutely necessary, is deducted few times, by various ways, and there is no way around.
I will keep this as it is, unless the tags will come more to play.
| local openj9_openjdk_tag_file="${BUILD_CONFIG[WORKSPACE_DIR]}/${BUILD_CONFIG[WORKING_DIR]}/${BUILD_CONFIG[OPENJDK_SOURCE_DIR]}/closed/openjdk-tag.gmk" | ||
| if [[ "${BUILD_CONFIG[BUILD_VARIANT]}" == "${BUILD_VARIANT_OPENJ9}" ]] && [[ -f "${openj9_openjdk_tag_file}" ]]; then | ||
| firstMatchingNameFromRepo=$(grep OPENJDK_TAG ${openj9_openjdk_tag_file} | awk 'BEGIN {FS = "[ :=]+"} {print $2}') | ||
| if [ "${BUILD_CONFIG[OPENJDK_FOREST_DIR]}" == "true" ]; then |
There was a problem hiding this comment.
Don't modify the getFirstTagFromOpenJDKGitRepo() method at all, it should only be called for a "git repo",
instead you need to modify the getOpenJdkVersion() to correctly return the jdk tag version of the forest openjdk source, I suspect as specified via the build arg --tag <jdk tag version>.
So I am thinking if --openjdk-source <path to tarball> is specified then --tag <jdk tag version> is mandatory in order to specify what the version within the source tarball is being built...
The tag version string is used by the build scripts for things like the image archive filenames...
There was a problem hiding this comment.
Don't modify the getFirstTagFromOpenJDKGitRepo() method at all, it should only be called for a "git repo", instead you need to modify the getOpenJdkVersion() to correctly return the jdk tag version of the forest openjdk
Thanx, will do!
source, I suspect as specified via the build arg --tag . So I am thinking if --openjdk-source is specified then --tag is mandatory in order to specify what the version within the source tarball is being built... The tag version string is used by the build scripts for things like the image archive filenames...
I think it defaults to some unknonw or head, but I will double check. If it do not default, then I would rather default it to some "local" rather then make it mandatory. Of course, it it is set, then it will be used
|
@judovana
Ping me if you have any questions |
Please dont, I would liek to go without any switch at all. The altid was optional safety belt, and will be removed, in favour of version (with adjsuted man page). The
I sitll hesitate on this. If tag would be specifed, then sure, it must be used. But without it, I would jsut default to "local" or similarly., rather then doing it mandatory.
Fair enough. As I described earlier, the
As this is removing any forms of I would liek to keep ti as it is. As this work is avoidign any git at all!
done! Tyvm for review ! |
|
You need to ensure whatever argument is specified, that you can determine: from them, as these must be set correctly so that the build scripts know what version specific stuff they need to do... With you forest source selection, no "git" operations should occur on the BUILD_CONFIG[WORKSPACE_DIR]/BUILD_CONFIG[WORKING_DIR]/BUILD_CONFIG[OPENJDK_SOURCE_DIR] |
I know. Tahts what ALTID was for. And now
Sure, will do. Still I'm wondering if it is not agasint he tide of "." of git clone. But sure! |
|
Just to confirm. Currently there are two proposed solutions of usecase: From those two I prefere first one, as it is less verbose. On contrary, it is giving the dir_or_repo dual meaning. |
We had small chat with @andrew-m-leonard and we decided that thsi feature can go in, once I rework it for As this will need major rework of the patch, I will close this PR and open new one |
|
surpassed by #3737 |
#3704
This is initial shot on implementation.
Wdyt?