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

Set build_full_name correctly when user build root directory specified #3722

Merged
merged 3 commits into from
Mar 24, 2024

Conversation

andrew-m-leonard
Copy link
Contributor

@andrew-m-leonard andrew-m-leonard commented Mar 21, 2024

configureBuild.sh specifies extra make args for the autoconf configuration for s390x, it unecessarily specifies
the CONF= parameter, which is not necessary as there is only one openjdk autoconf configuration, and
was incorrect when a user build root directory is specified.

build_full_name is the OpenJDK autoconf configuration name which is set to:

  • When building within the OpenJDK src root:
$openjdk src root> bash ./configure ...    => build output to => build/linux-x86_64-server-release
  • When building within a user specified build output directory:
$/user_directory> bash /path_to_openjdk_src/configure ...  => build output to => /user_directory

jdk8u s390x test build: https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk8u/job/jdk8u-linux-s390x-temurin/296/
jdk11u s390x test build: https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk11u/job/jdk11u-linux-s390x-temurin/329/
jdk21u s390x test build: https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk21u/job/jdk21u-linux-s390x-temurin/105/

@andrew-m-leonard andrew-m-leonard self-assigned this Mar 21, 2024
@github-actions github-actions bot added the testing Issues that enhance or fix our test suites label Mar 22, 2024
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.

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.

This is a follow on to #3696 which I haven't fully reviewed.

My initial concern was that this could change th4e defaults to squash the variant part of the build output directory in some cases, but I can't see that it's being included anyway so I don't think this will have any side effects. On that basis I'm approving although I'd say "cautiously" sine I don't know why the CONF= was added in the first place.

Since I looked for the source, the reason for these additions being in there appear to have been lost in the depths of time as it was in a [giant rewrite of the codebase](https://github.com/adoptium/temurin-build/pull/393/files#diff-070a0531721fb82ff7f28fa30a025aa94f9de4db08619d0ee22c3325093cd4ec and the switch for JDK versions >=12 was added in this commit)

@sxa
Copy link
Member

sxa commented Mar 22, 2024

Tagging @johnoliver @gdams @karianna in case any of them know anything about the original inclusion of CONF= in here

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@andrew-m-leonard
Copy link
Contributor Author

andrew-m-leonard commented Mar 22, 2024

This is a follow on to #3696 which I haven't fully reviewed.

My initial concern was that this could change th4e defaults to squash the variant part of the build output directory in some cases, but I can't see that it's being included anyway so I don't think this will have any side effects. On that basis I'm approving although I'd say "cautiously" sine I don't know why the CONF= was added in the first place.

Since I looked for the source, the reason for these additions being in there appear to have been lost in the depths of time as it was in a [giant rewrite of the codebase](https://github.com/adoptium/temurin-build/pull/393/files#diff-070a0531721fb82ff7f28fa30a025aa94f9de4db08619d0ee22c3325093cd4ec and the switch for JDK versions >=12 was added in this commit)

@sxa yep, I search back through git blame history to no avail... but it was only for s390x, I am wondering if it was added when maybe building both a "zero" and a "server" variant within the same build...?
We could keep the logic and switch from either CONF=X or SPEC=<userbuilddirectory>, but I really couldn't see why we need it....

@andrew-m-leonard andrew-m-leonard dismissed github-actions[bot]’s stale review March 22, 2024 14:56

no block on master branch now

@karianna
Copy link
Contributor

I don't have recollection of this variable, so let's merge for now.

@karianna karianna merged commit 17b50e9 into adoptium:master Mar 24, 2024
22 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants