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

8253424: Add support for running pre-submit testing using GitHub Actions #284

Closed
wants to merge 8 commits into from

Conversation

rwestberg
Copy link
Member

@rwestberg rwestberg commented Sep 21, 2020

A few days ago I posted an initial version of the necessary configuration required to run pre-submit build and tests for JDK main-line contributions using GitHub Actions [2] and the free tier [3] available to everyone working with open source repositories. I've incorporated the feedback into an updated version that I believe is ready to be integrated.

If this is integrated into the master branch, future branches created and updated in personal forks will build and run the basic tier 1 tests as described in this configuration, on Linux, Windows and macOS (all on x64). It's of course possible for any contributor to opt out fully or partially of these automatic runs in a few different ways.

To opt out completely, a contributor can simply disable GitHub Actions on their personal fork, and no further jobs will be executed. Another option is to add a repository secret [4] with the name JDK_SUBMIT_FILTER set to any value. If this is set, only branches prefixed with submit/ will be subject to automatic build and test. This can also be further refined by adding a repository secret named JDK_SUBMIT_PLATFORMS with a value such as Linux x64, Windows x64 to limit automatic build and test to these two platforms. It will still be possible to run the tests on any branch and/or platform by manually triggering the workflow.

To see what this looks like in practice, an example run can be found here: https://github.com/rwestberg/jdk/actions/runs/265131985 (note that there is currently a failing test on Windows which is tracked by JDK-8249095, which should probably be resolved before this change is integrated).

Best regards,
Robin

[1] https://mail.openjdk.java.net/pipermail/jdk-dev/2020-September/004736.html
[2] https://github.com/features/actions
[3] https://docs.github.com/en/actions/getting-started-with-github-actions/about-github-actions#usage-limits
[4] https://docs.github.com/en/actions/reference/encrypted-secrets


Progress

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

Issue

  • JDK-8253424: Add support for running pre-submit testing using GitHub Actions

Reviewers

Download

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

@rwestberg rwestberg changed the title Add support for running pre-submit testing using GitHub Actions 8253424: Add support for running pre-submit testing using GitHub Actions Sep 21, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 21, 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 added the rfr label Sep 21, 2020
@rwestberg
Copy link
Member Author

@rwestberg rwestberg commented Sep 21, 2020

/label add build

@openjdk openjdk bot added the build label Sep 21, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2020

@rwestberg
The build label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 21, 2020

Webrevs

@erikj79
Copy link
Member

@erikj79 erikj79 commented Sep 21, 2020

Is there any way to derive the version strings for dependencies from a different source? Once pushed, this file will require updating whenever the bootjdk, jtreg version or gtest version is updated.

@rwestberg
Copy link
Member Author

@rwestberg rwestberg commented Sep 21, 2020

Certainly, the prerequisites step can checkout additional repositories and run shell commands to extract variables to be used for later steps. Do we have any suitable source for these versions? We'll also need download links for the different flavors of the bootjdk (and ideally the sha256 hash, although the verification step could perhaps be skipped).

@erikj79
Copy link
Member

@erikj79 erikj79 commented Sep 21, 2020

We don't have a good source currently no, but something to keep in mind. Ideally we would want to create a common source for both jib-profiles.js and the gitactions to read from. I would prefer something that can be read both as a properties file and sourced as a shell script, much like the version-numbers file currently is. That could be done as a separate change though.

@rwestberg
Copy link
Member Author

@rwestberg rwestberg commented Sep 22, 2020

Sure, should not be that hard to parse something similar. The GitHub actions will probably need it in JSON format at some point, but nothing a little sed -e '1i {' -e 's/#.*//g' -e 's/"//g' -e 's/\(.*\)=\(.*\)/"\1": "\2",/g' -e '$s/,\s\{0,\}$/\}/' won't solve. Any suggestion for the location and name of such a file? I'm thinking it would contain entries like this:

BOOT_JDK_VERSION="14.0.2"
JTREG_VERSION="jtreg5.1-b01"
GTEST_VERSION="release-1.8.1"
LINUX_X64_BOOT_JDK_FILENAME="openjdk-14.0.2_linux-x64_bin.tar.gz"
LINUX_X64_BOOT_JDK_URL="https://download.java.net/java/GA/jdk14.0.2/205943a0976c4ed48cb16f1043c5c647/12/GPL/openjdk-14.0.2_linux-x64_bin.tar.gz"
LINUX_X64_BOOT_JDK_SHA256="91310200f072045dc6cef2c8c23e7e6387b37c46e9de49623ce0fa461a24623d"

@erikj79
Copy link
Member

@erikj79 erikj79 commented Sep 22, 2020

The other primary consumer of this is make/conf/jib-profiles.js. The make/conf dir would make sense to me.

The challenge here is creating a set of variables that are suitable enough for both config files to consume. For BootJDK, we never bothered with bumping the version for updates, and I very much doubt we will do that in the future for github actions, so a plain major version 14, and soon 15, would be preferred from my point of view. This is however not enough for jib-profiles.js (yet) so we can't really share bootjdk config for now anyway. For jtreg, we specify 5.1 and b01 as two separate metadata values. For gtest we specify the version as 1.8.1.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2020

@rwestberg To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request.

In order to have an RFR email automatically sent to the correct mailing list, you will need to add one or more labels manually using the /label add "label" command. The following labels are valid: 2d awt beans build compiler core-libs hotspot hotspot-compiler hotspot-gc hotspot-jfr hotspot-runtime i18n javadoc jdk jmx kulla net nio security serviceability shenandoah sound swing.

@rwestberg
Copy link
Member Author

@rwestberg rwestberg commented Sep 23, 2020

I added make/conf/test-dependencies with version numbers specified on the format that jib-profiles.js would expect. Actually using them from that file as well could perhaps be a separate task though. :)

@erikj79
Copy link
Member

@erikj79 erikj79 commented Sep 23, 2020

The version-numbers file (which is also a shared properties style file) is not using quotes for values, which is fine as long as there are no spaces. I believe if you read it as a properties file, you need to strip the quotes if you have them. I prefer if version-numbers and test-dependencies using the same format.

Otherwise this looks good to me!

@rwestberg
Copy link
Member Author

@rwestberg rwestberg commented Sep 23, 2020

Sounds reasonable, didn't notice that discrepancy! Fixed in the latest commit.

@rwestberg
Copy link
Member Author

@rwestberg rwestberg commented Sep 25, 2020

After some feedback on the jdk-dev mailing list (thanks @jaikiran!) I took a second look at removing artifacts that are only created for passing between the build and test steps. Turns out that there is an API for this (it's still in preview, but seems to work fine) that can be used for this. When this API is finalized we can update that part, but it's not a deal-breaker even if it should change, as the test runs will still complete successfully. With this API available, it's also possible to publish a single artifact containing all test results, regardless of outcome.

edvbld
edvbld approved these changes Sep 25, 2020
Copy link
Member

@edvbld edvbld left a comment

Looks good to me, awesome work @rwestberg!

@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 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 more details.

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

8253424: Add support for running pre-submit testing using GitHub Actions

Reviewed-by: ehelin, erikj

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 84 new commits pushed to the master branch:

  • 8e87d46: 8252857: AArch64: Shenandoah C1 CAS is not sequentially consistent
  • c2692f8: 8225329: -XX:+PrintBiasedLockingStatistics causes crash during initia…
  • e9c1782: 8252752: Clear card table for old regions during scan in G1
  • 276fcee: 8252835: Revert fix for JDK-8246051
  • ca1ed16: 8253639: Change os::attempt_reserve_memory_at parameter order
  • fed3636: 8252219: C2: Randomize IGVN worklist for stress testing
  • 625a935: 8253638: Cleanup os::reserve_memory and remove MAP_FIXED
  • f014854: 8253624: gtest fails when run from make with read-only source directory
  • 7817963: 8247691: [aarch64] Incorrect handling of VM exceptions in C1 deopt stub/traps
  • 79904c1: 8252730: jlink does not create reproducible builds on different servers
  • ... and 74 more: https://git.openjdk.java.net/jdk/compare/7e49eaecbcea46f61b30066112b68b04af7e5590...master

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 label Sep 25, 2020
@openjdk openjdk bot deleted a comment from openjdk bot Sep 28, 2020
@rwestberg
Copy link
Member Author

@rwestberg rwestberg commented Sep 28, 2020

/integrate

@openjdk openjdk bot closed this Sep 28, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 28, 2020

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

  • 8e87d46: 8252857: AArch64: Shenandoah C1 CAS is not sequentially consistent
  • c2692f8: 8225329: -XX:+PrintBiasedLockingStatistics causes crash during initia…
  • e9c1782: 8252752: Clear card table for old regions during scan in G1
  • 276fcee: 8252835: Revert fix for JDK-8246051
  • ca1ed16: 8253639: Change os::attempt_reserve_memory_at parameter order
  • fed3636: 8252219: C2: Randomize IGVN worklist for stress testing
  • 625a935: 8253638: Cleanup os::reserve_memory and remove MAP_FIXED
  • f014854: 8253624: gtest fails when run from make with read-only source directory
  • 7817963: 8247691: [aarch64] Incorrect handling of VM exceptions in C1 deopt stub/traps
  • 79904c1: 8252730: jlink does not create reproducible builds on different servers
  • ... and 74 more: https://git.openjdk.java.net/jdk/compare/7e49eaecbcea46f61b30066112b68b04af7e5590...master

Your commit was automatically rebased without conflicts.

Pushed as commit 840aa2b.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 28, 2020

Mailing list message from Magnus Ihse Bursie on build-dev:

On 2020-09-23 19:27, Erik Joelsson wrote:

The version-numbers file (which is also a shared properties style file) is not using quotes for values, which is fine
as long as there are no spaces. I believe if you read it as a properties file, you need to strip the quotes if you have
them. I prefer if version-numbers and test-dependencies using the same format.

I've been away for a while, but now I'm back -- with a royal backlog. :)

On a related note, I *really* think we should move the version-numbers
file to make/conf. I think I even have an open bug on that somewhere.
The move itself is trivial, but the consequences needs to be checked
carefully.

I'm happy that you put the new test-dependencies file in make/conf, though!

/Magnus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build integrated
3 participants