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

8256393: Github Actions build on Linux should define OS and GCC versions #1225

Closed
wants to merge 3 commits into from

Conversation

rwestberg
Copy link
Member

@rwestberg rwestberg commented Nov 16, 2020

We should be more explicit about OS and compiler versions used in the GitHub Actions builds, to avoid problems caused by unexpected changes to the defaults. This patch changes the OS and GCC versions used from ubuntu-latest (currently 18.04, but will change to 20.04 sometime soon) / default (currently 9.3.0) to 20.04 / 10.2.0.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux aarch64 Linux arm Linux ppc64le Linux s390x Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8256393: Github Actions build on Linux should define OS and GCC versions

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1225/head:pull/1225
$ git checkout pull/1225

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 16, 2020

👋 Welcome back rwestberg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title 8256393 8256393: Github Actions build on Linux should define OS and GCC versions Nov 16, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 16, 2020
@openjdk
Copy link

openjdk bot commented Nov 16, 2020

@rwestberg The following label will be automatically applied to this pull request:

  • build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the build build-dev@openjdk.org label Nov 16, 2020
@mlbridge
Copy link

mlbridge bot commented Nov 16, 2020

Webrevs

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

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

Hold on a sec. ubuntu-latest is ubuntu-18.04, as per GH manual. So this effectively upgrades the whole thing to Ubuntu 20.04, and upgrades GCC then? I think we better stick to current defaults, i.e. ubuntu-18.04 and its GCC.

In JDK-8256277, we did not upgrade MacOS target either...

@@ -88,7 +88,7 @@ jobs:
if: steps.check_submit.outputs.should_run != 'false' && steps.jtreg.outputs.cache-hit != 'true'

- name: Build jtreg
run: sh make/build-all.sh ${JAVA_HOME}
run: sh make/build-all.sh ${JAVA_HOME_8_X64}
Copy link
Member

Choose a reason for hiding this comment

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

What is this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

On ubuntu-20.04 the default Java installation is now set to 11, but jtreg still requires Java 8 for building.

Copy link
Member

Choose a reason for hiding this comment

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

I see! Okay then.

@rwestberg
Copy link
Member Author

Right, currently ubuntu-latest means 18.04, but that is only true for another two weeks (see actions/runner-images#1816 - originally it was also planned for next week). So I think we should go straight for the upcoming latest.

@@ -88,7 +88,7 @@ jobs:
if: steps.check_submit.outputs.should_run != 'false' && steps.jtreg.outputs.cache-hit != 'true'

- name: Build jtreg
run: sh make/build-all.sh ${JAVA_HOME}
run: sh make/build-all.sh ${JAVA_HOME_8_X64}
Copy link
Member

Choose a reason for hiding this comment

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

I see! Okay then.

@openjdk
Copy link

openjdk bot commented Nov 16, 2020

@rwestberg This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8256393: Github Actions build on Linux should define OS and GCC versions

Reviewed-by: shade, erikj, ihse

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 80 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 16, 2020
@shipilev
Copy link
Member

Right, currently ubuntu-latest means 18.04, but that is only true for another two weeks (see actions/virtual-environments#1816 - originally it was also planned for next week). So I think we should go straight for the upcoming latest.

All right, changing this today makes sense to me.

run: sudo apt-get install libxrandr-dev libxtst-dev libcups2-dev libasound2-dev
run: |
sudo apt-get install libxrandr-dev libxtst-dev libcups2-dev libasound2-dev
sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-10 100 --slave /usr/bin/g++ g++ /usr/bin/g++-10
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use apt-get functionality to install a specific version of packages? I'm not sure how relevant it is for the X and alsa libraries since they change very seldom, but perhaps for gcc, to get a specific point release of the compiler.

Copy link
Member

@magicus magicus Nov 16, 2020

Choose a reason for hiding this comment

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

Something along the lines of sudo apt-get install gcc-10=10.2.0-5ubuntu1~20, which I believe should match quite well the version used internally in the Oracle CI builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds reasonable, I don't know how often these change in Ubuntu LTS, but can't hurt to be explicit here as well.

@mlbridge
Copy link

mlbridge bot commented Nov 16, 2020

Mailing list message from Tiago Daitx on build-dev:

Hi,

89:
90: - name: Build jtreg
91: run: sh make/build-all.sh ${JAVA_HOME_8_X64}

What is this change?

On ubuntu-20.04 the default Java installation is now set to 11, but jtreg still requires Java 8 for building.

Why does jtreg need OpenJDK-8 for building? On Debian/Ubuntu we have
been using the 'release' parameter to build jtreg using OpenJDK 11
while maintaining OpenJDK 8 compatibility. See
https://sources.debian.org/src/jtreg/5.1-b01-2/debian/patches/use-release-instead-of-source-target.patch/
for how we do it.

Please note that while we replace "source/target" for "release", one
can specify both "source/target" and "release" for a javac task: ant
is smart enough to pick one and ignore the other, depending on it
being run on JDK9+ or not. See 'release' description at
https://ant.apache.org/manual/Tasks/javac.html

Cheers!

--
Tiago St?rmer Daitx
Software Engineer
tiago.daitx at canonical.com

PGP Key: 4096R/F5B213BE (hkp://keyserver.ubuntu.com)
Fingerprint = 45D0 FE5A 8109 1E91 866E 8CA4 1931 8D5E F5B2 13BE

@rwestberg
Copy link
Member Author

Why does jtreg need OpenJDK-8 for building?

Fair enough, the 1.8 requirement comes from using build-all.sh which checks this explicitly. But I'd rather leave changing how we build jtreg for a separate change..

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

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

LGTMN.

@rwestberg
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Nov 20, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 20, 2020
@openjdk
Copy link

openjdk bot commented Nov 20, 2020

@rwestberg Since your change was applied there have been 90 commits pushed to the master branch:

  • 5fedb69: 8250888: nsk/jvmti/scenarios/general_functions/GF08/gf08t001/TestDriver.java fails
  • 02adaa5: 8255885: Metaspace: freelist commit counter is not updated when purging
  • fa240f2: 8256594: Unexpected warning: SIGSEGV handler flags expected:SA_RESTART|SA_SIGINFO found:SA_RESTART|SA_SIGINFO
  • 4c09525: 8256108: Create implementation for NSAccessibilityElement protocol peer
  • 6813889: 8251317: Support for CLDR version 38
  • c816464: 4916923: In MetalRootPaneUI, MetalRootLayout does not correctly calculate minimumsize
  • fae68ff: 8256640: assert(!m->is_old() || ik()->is_being_redefined()) failed: old methods should not be in vtable
  • c140773: 8256692: Zero: remove obsolete block from ZeroInterpreter::native_entry
  • 080c707: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly
  • b9db002: 8256682: JDK-8202343 is incomplete
  • ... and 80 more: https://git.openjdk.java.net/jdk/compare/8eeb36f14a9121b6cb1ed3228f78021d5da9e81b...master

Your commit was automatically rebased without conflicts.

Pushed as commit c45ab1a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants