enabled build from source dir/tarball via to --openjdk-source-location/-l #3737

judovana · 2024-03-28T08:30:44Z

Surpassing #3724

not using per-jdk-major build dirs, using src, or whatever user set up
always cleaning src (or whatever was set up) dir
do not remove config.status, but instead cleaning, and not copying/unpacking ./build
do no tmodify getFirstTagFromOpenJDKGitRepo, modified getOpenJdkVersion instead
not enforcing the --tag but warning if it is not present. Reusing default tag if not set (which works pretty well)
kept the relative paths, as it is what all the functions in build epxects. Will change to absolute if it will be insisted on
It do not yet support build in container (although I would like to add)
should be jsut change in genrated docker file from clone to cp in?
works above both dirs and tarballs

Wdyt?

judovana · 2024-03-28T09:57:57Z

Yet again the label is wrong....

@andrew-m-leonard wdyt about this version?

sbin/build.sh

sbin/common/config_init.sh

sbin/prepareWorkspace.sh

andrew-m-leonard · 2024-03-28T14:47:51Z

sbin/prepareWorkspace.sh

+copyFromDir() {
+  echo "Copyng existing ${BUILD_CONFIG[OPENJDK_FOREST_DIR_ABSPATH]} to `pwd`/${BUILD_CONFIG[OPENJDK_SOURCE_DIR]} to be built"
+  # we really do not want to use .git for dirs, as we expect user have them set up, ignoring them
+  local files=$(find ${BUILD_CONFIG[OPENJDK_FOREST_DIR_ABSPATH]} -maxdepth 1 -mindepth 1 | grep -v -e "/workspace$" -e "/build$" -e "/.git" -e -"/build/")


Not sure about the ignores for workspace, build, .git, ...., this list could grow or become wrong...
I think we should assume the source tarball is exactly that, if someone puts something in their that is "wrong" then we are not really judging if it is right or wrong....?

I Agree it may be ragile, however it have its reasons.

.git is useless, we want to build it as it is.Actually its presence may be disturbing

in addition, .git is quite big,and by nature of its content very slow to copy.

build is important, it is big, and it would need to be deleted anyway

in addition it is default of jdk build system for ages.

As for workspace, it is tricky. In temurin build, can workspace dir be renamed/relocated?
It have to be excluded- otherwise, when;

cd ~/git/myJdkClone sh ~/git/temurin-build/makejdk-any-platform.sh -o . jdk21u

Then the worskapce will copy itself.
I had not found the renaming switch/setup. I had overlooked most liekly, so I hardcoded it.

I was not thinking what is wrong or not. Excluding of workspace is a must. Excluding of a .git is just good. And the build needs to go away anyway in worksapce.... If there will be more excludes on time, just good. Maybe I it should have been made customizable?

Teh source tarball is most likely ok. But in case of local directory? That will always contain at least .git. And in many cases also the workspace and build:(

sbin/prepareWorkspace.sh

judovana · 2024-03-28T15:40:25Z

Thanx a lot for reading it out. I had fixed all what coudl be fixed without asking. Two topic remained above.

karianna · 2024-04-02T07:59:25Z

@judovana Some linter issues

makejdk-any-platform.1

sbin/prepareWorkspace.sh

sxa

Some initial comments - I might come back and add more as I haven't followed all of the logic through yet. Ref the tar comments - these scripts are designed to be used on non-GNU environments like AIX and Solaris which may not have gnu tar installed, or not have them as the default tar implementation.
Having said that, it won't affect the Temurin pipelines in jenkins since they won't be using this option ;-)

makejdk-any-platform.1

sbin/prepareWorkspace.sh

sbin/build.sh

sbin/common/config_init.sh

judovana · 2024-04-03T12:54:54Z

@sxa

I'd personally go with Andrew's suggestion this case, but I wouldn't insist on it so 🤷🏻 I'll let you both decide ;-)

Then I will keep the flag and value separaed. If @andrew-m-leonard will insist, I will go with value only (wihtout any additional talking).

This is the name specifically of a source archive right? So maybe the name should reflect that - somethig like OPENJDK_FOREST_SOURCE_ARCHIVE might be better.

The main target actually is local directory, and when I was doing this for makejdk-any-platform.sh I implemented dir first, and added source tarball later. The proper OPENJDK_FOREST_DIR_OR_SOURCE_ARCHIVE is terrible. Thus I would vote to keep shorter OPENJDK_FOREST_DIR but If you have strong prefference I do not mind to preffer source archive bit and go with OPENJDK_FOREST_SOURCE_ARCHIVE or even that superlong OR version...

redundant if [ "${BUILD_CONFIG[OPENJDK_FOREST_DIR]}" == "true" ] from getFirstTagFromOpenJDKGitRepo renamed setLocalDir to setOpenjdkSourceDir wrong parent of removed build dir after unpack bounch of typos: does not exists Copying OpenJDK source from Extracting OpenJDK source tarbal contains, does not contain

thanx shell lint

Co-authored-by: Martijn Verburg <[email protected]>

sbin/build.sh

karianna · 2024-04-23T10:52:48Z

sbin/common/config_init.sh

@@ -81,6 +81,8 @@ OPENJDK_BUILD_NUMBER
 OPENJDK_CORE_VERSION
 OPENJDK_FEATURE_NUMBER
 OPENJDK_FOREST_NAME
+OPENJDK_FOREST_SOURCE_ARCHIVE


Archive to me usually means .zip or .tar etc. Is OPENJDK_FOREST_SOURCE or something OPENJDK_LOCAL_SOURCE a better name?

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.

sh makejdk-any-platform.sh -o ~/git/java-21-openjdk-portable/openjdk-21.0.2+13.tar.xz jdk21u

and

sh makejdk-any-platform.sh -o ~/git/jdk21u/ jdk21u

both do same job.

actually, OPENJDK_LOCAL_SOURCE and OPENJDK_LOCAL_SOURCE_ABSPATH are generic enough. @sxa , I will use that.

…RCHIVE Tus resulting for both usages: -OPENJDK_FOREST_SOURCE_ARCHIVE -OPENJDK_FOREST_SOURCE_ARCHIVE_ABSPATH +OPENJDK_LOCAL_SOURCE_ARCHIVE +OPENJDK_LOCAL_SOURCE_ARCHIVE_ABSPATH

karianna

Broadly LGTM - over to @sxa

judovana · 2024-04-23T11:27:56Z

Broadly LGTM - over to @sxa

TY!

andrew-m-leonard · 2024-05-01T10:15:01Z

/thaw

Pull Request unblocked - code freeze is over.

andrew-m-leonard · 2024-05-01T10:16:20Z

macOS jdk11u CI failure unrelated, and already fixed in master

sxa · 2024-05-09T14:20:34Z

Maybe we don't need the reset counter in this case but that's a minor point :-)

Running some tests just now and will look through the code while I'm doing that.

andrew-m-leonard

looks good

andrew-m-leonard · 2024-05-09T14:52:09Z

/thaw

sxa

A few comments/suggestions/discussion points before approving but overall this LGTM and I have no major objections.
I'd normally be more strict on it but if you want to leave the GNU-isms in place I'd be ok in this situation since it's not functionality that the temurin pipelines will use, and the platforms that will be affected are, how should I put it, "niche" ;-)

makejdk-any-platform.1

sbin/build.sh

sbin/common/config_init.sh

sbin/prepareWorkspace.sh

sxa · 2024-05-09T14:56:12Z

sbin/prepareWorkspace.sh

+      tar --strip-components 0 -xf "${BUILD_CONFIG[OPENJDK_FOREST_DIR_ABSPATH]}"
+    fi
+    rm -rf "build"
+  popd


Not seen that before, but I'm ok with it :-)

sxa · 2024-05-09T15:05:26Z

sbin/prepareWorkspace.sh

@@ -79,6 +122,8 @@ checkoutAndCloneOpenJDKGitRepo() {
      echo "If this is inside a docker you can purge the existing source by passing --clean-docker-build"
      exit 1
    fi
+  elif [ "${BUILD_CONFIG[OPENJDK_LOCAL_SOURCE_ARCHIVE]}" == "true" ]; then
+    copyFromDirOrUnpackFromArchive


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.

ok. let me try.

Oh I see. There is

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" ] && [ "${BUILD_CONFIG[OPENJDK_LOCAL_SOURCE_ARCHIVE]}" == "false" ]; then

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 :

function configureWorkspace() { if [[ "${BUILD_CONFIG[ASSEMBLE_EXPLODED_IMAGE]}" != "true" ]]; then createWorkspace downloadingRequiredDependencies downloadDevkit relocateToTmpIfNeeded +if [ "${BUILD_CONFIG[OPENJDK_LOCAL_SOURCE_ARCHIVE]}" == "true" ]; then + copyFromDirOrUnpackFromArchiveWrapper +else checkoutAndCloneOpenJDKGitRepo +fi applyPatches if [ "${BUILD_CONFIG[CUSTOM_CACERTS]}" = "true" ] ; then prepareMozillaCacerts fi fi writeDockerImageSHA }

where the wrapper:

function copyFromDirOrUnpackFromArchiveWrapper() { cd "${BUILD_CONFIG[WORKSPACE_DIR]}/${BUILD_CONFIG[WORKING_DIR]}" copyFromDirOrUnpackFromArchive cd "${BUILD_CONFIG[WORKSPACE_DIR]}" }

IN addition the method of checkoutAndCloneOpenJDKGitRepo is doing a bit more before cd "${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 them

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 :-)

Co-authored-by: Stewart X Addison <[email protected]>

Co-authored-by: Martijn Verburg <[email protected]>

Co-authored-by: Stewart X Addison <[email protected]>

usabel also for zips if needed Added fogooten quotes

… | "-l"

judovana · 2024-05-10T15:14:24Z

@sxa I had elaborated on all your comments I had found. Please let me know if you need anything more. Thanx a lto for suggestions!

sxa

On the basis that all of my comments have been addressed I'll approve this.
I'll let you make the decision on whether to do the extra change to move things into configureWorkspace - I suggest we hold of merging this until Monday (Putting something like this on a Friday always seems like a bad idea!)

judovana · 2024-05-10T15:43:42Z

sure. TY!

…from -o

judovana force-pushed the localBuild3 branch from b1b2b93 to 93e62b4 Compare March 28, 2024 08:37

github-actions bot added the docker Issues related to our docker files and docker scripts label Mar 28, 2024

judovana mentioned this pull request Mar 28, 2024

allowing build from diectory/src snapshot #3724

Closed

andrew-m-leonard added enhancement Issues that enhance the code or documentation of the repo in any way and removed docker Issues related to our docker files and docker scripts labels Mar 28, 2024

andrew-m-leonard assigned judovana Mar 28, 2024

andrew-m-leonard reviewed Mar 28, 2024

View reviewed changes

github-actions bot added the docker Issues related to our docker files and docker scripts label Mar 28, 2024

karianna requested a review from andrew-m-leonard March 30, 2024 07:24

fitzsim mentioned this pull request Mar 30, 2024

comparable_patch.sh: Zero out .gnu_debuglink CRCs (#3730) #3736

Merged

karianna requested changes Apr 3, 2024

View reviewed changes

judovana requested a review from karianna April 3, 2024 11:37

sxa reviewed Apr 3, 2024

View reviewed changes

makejdk-any-platform.1 Outdated Show resolved Hide resolved

sbin/prepareWorkspace.sh Outdated Show resolved Hide resolved

sbin/build.sh Show resolved Hide resolved

sbin/common/config_init.sh Outdated Show resolved Hide resolved

sbin/common/config_init.sh Outdated Show resolved Hide resolved

judovana requested a review from sxa April 3, 2024 12:56

judovana and others added 12 commits April 5, 2024 18:29

enabled build from source dir/tarball via to --openjdk-source

a635458

not excluding build from tarballs extraction, rmeoving instead

b375a2a

removed debug

70278b4

Quoting few variables to prevent globbing and word splitting

d7a1ef3

thanx shell lint

Replaced pwd by $(pwd)

433ea0f

Update makejdk-any-platform.1

0620e50

Co-authored-by: Martijn Verburg <[email protected]>

Update makejdk-any-platform.1

8cb6d80

Co-authored-by: Martijn Verburg <[email protected]>

Update makejdk-any-platform.1

e0ccb12

Co-authored-by: Martijn Verburg <[email protected]>

Update sbin/prepareWorkspace.sh

6c1bafb

Co-authored-by: Martijn Verburg <[email protected]>

Update sbin/prepareWorkspace.sh

7030bcd

Co-authored-by: Martijn Verburg <[email protected]>

Update sbin/prepareWorkspace.sh

eabc2c2

Co-authored-by: Martijn Verburg <[email protected]>

karianna requested changes Apr 23, 2024

View reviewed changes

using OPENJDK_LOCAL_SOURCE_ARCHIVE instead of OPENJDK_FOREST_SOURCE_A…

43b5c93

…RCHIVE Tus resulting for both usages: -OPENJDK_FOREST_SOURCE_ARCHIVE -OPENJDK_FOREST_SOURCE_ARCHIVE_ABSPATH +OPENJDK_LOCAL_SOURCE_ARCHIVE +OPENJDK_LOCAL_SOURCE_ARCHIVE_ABSPATH

karianna approved these changes Apr 23, 2024

View reviewed changes

Merge branch 'master' into localBuild3

45e4efb

Merge branch 'master' into localBuild3

23454cc

andrew-m-leonard approved these changes May 9, 2024

View reviewed changes

sxa reviewed May 9, 2024

View reviewed changes

judovana and others added 9 commits May 10, 2024 15:51

Update sbin/common/config_init.sh

5e857d3

Co-authored-by: Stewart X Addison <[email protected]>

Update sbin/build.sh

d8304b9

Co-authored-by: Martijn Verburg <[email protected]>

Update makejdk-any-platform.1

faf2bea

Co-authored-by: Stewart X Addison <[email protected]>

Update makejdk-any-platform.1

72d3ed9

Co-authored-by: Stewart X Addison <[email protected]>

Update makejdk-any-platform.1

a399523

Co-authored-by: Stewart X Addison <[email protected]>

Made tar --exclude check nonfatal

22299e6

SC2126 Considered using 'grep -c' instead of 'grep|wc -l'.

9140925

Workaroudned usage of --strip-components 1 by custom function

7edc874

usabel also for zips if needed Added fogooten quotes

renamed from "--openjdk-source" | "-o" to "--openjdk-source-location"…

5dcb140

… | "-l"

judovana force-pushed the localBuild3 branch from d37941c to 5dcb140 Compare May 10, 2024 15:04

Using $() isntead of backticks

385bfc3

sxa approved these changes May 10, 2024

View reviewed changes

judovana changed the title ~~enabled build from source dir/tarball via to --openjdk-source~~ enabled build from source dir/tarball via to --openjdk-source-location/-l May 11, 2024

Moved -l/openjdk-source-location to proper palce in man after rename …

793fcad

…from -o

sxa merged commit cd2778d into adoptium:master May 13, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

judovana commented Mar 28, 2024 •

edited

Loading

judovana commented Mar 28, 2024

andrew-m-leonard Mar 28, 2024

judovana Mar 28, 2024 •

edited

Loading

judovana Mar 28, 2024

judovana commented Mar 28, 2024

karianna commented Apr 2, 2024

sxa left a comment

judovana commented Apr 3, 2024 •

edited

Loading

karianna Apr 23, 2024

judovana Apr 23, 2024

judovana Apr 23, 2024 •

edited

Loading

karianna left a comment

judovana commented Apr 23, 2024

andrew-m-leonard commented May 1, 2024

andrew-m-leonard commented May 1, 2024 •

edited

Loading

sxa commented May 9, 2024

andrew-m-leonard left a comment

andrew-m-leonard commented May 9, 2024

sxa left a comment

sxa May 9, 2024

sxa May 9, 2024

judovana May 10, 2024

judovana May 10, 2024 •

edited

Loading

sxa May 10, 2024

judovana commented May 10, 2024

sxa left a comment

judovana commented May 10, 2024

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

Conversation

judovana commented Mar 28, 2024 • edited Loading

judovana commented Mar 28, 2024

Choose a reason for hiding this comment

judovana Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judovana commented Mar 28, 2024

karianna commented Apr 2, 2024

sxa left a comment

Choose a reason for hiding this comment

judovana commented Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judovana Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

karianna left a comment

Choose a reason for hiding this comment

judovana commented Apr 23, 2024

andrew-m-leonard commented May 1, 2024

andrew-m-leonard commented May 1, 2024 • edited Loading

sxa commented May 9, 2024

andrew-m-leonard left a comment

Choose a reason for hiding this comment

andrew-m-leonard commented May 9, 2024

sxa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judovana May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judovana commented May 10, 2024

sxa left a comment

Choose a reason for hiding this comment

judovana commented May 10, 2024

judovana commented Mar 28, 2024 •

edited

Loading

judovana Mar 28, 2024 •

edited

Loading

judovana commented Apr 3, 2024 •

edited

Loading

judovana Apr 23, 2024 •

edited

Loading

andrew-m-leonard commented May 1, 2024 •

edited

Loading

judovana May 10, 2024 •

edited

Loading