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

8254691: Enable GitHub actions for jfx repo #338

Closed

Conversation

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 24, 2020

This is a proposed fix for JDK-8254691 to enable GitHub actions for the jfx repo. It is similar in purpose to what was done for the jdk repo.

Once this is integrated, and subsequently merged into your personal fork, a GitHub Actions workflow will run on every push to any branch in your personal fork except a production branch (so that syncing the upstream master or jfx* branch won't cause a build) or any branch whose name that starts with WIP.

The workflow does a build and (headless) test run on all three platforms: Linux, macOS, and Windows. The build is done without building the native media or WebKit libraries. Web tests are excluded.

Here is the job that resulted from the most recent push to the 8254691-github-actions branch in my personal fork:

https://github.com/kevinrushforth/jfx/actions/runs/329355746

The workflow checks out the sources for the branch, downloads the boot JDK, installs the needed software packages, and then runs two build steps, gradlew all followed by gradlew test -x :web:test.

The Skara tooling will process the results of the GitHub actions run, and show them in the Testing section of a pull request, as you can see below.

Follow-on work:

  1. Get BOOT JDK parameters from build.properties. Currently the location of the bootJDK is hardcoded in the submit.yml file. This should be obtained from build.properties; we might need to add a couple more properties.

  2. Check the shasum of the downloaded bootJDK and enable caching of the JDK (in order to save download time). This requires step 1 to be done first.

  3. Locate the Visual Studio compiler on Windows. The location and version of the Microsoft Visual Studio 2019 compiler is hard-coded which will break if and when the GitHub build farm updates their Windows system. This will require making our build itself a little smarter, possibly using vswhere.


Progress

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

Testing

Linux x64 Windows x64 macOS x64
Build / test ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed)

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/338/head:pull/338
$ git checkout pull/338

Initial working version on Linux, Mac, Windows
@kevinrushforth kevinrushforth self-assigned this Oct 24, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 24, 2020

👋 Welcome back kcr! 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.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Oct 24, 2020

@rwestberg I was pleasantly surprised to see the Skara bot add this:

  Linux x64 Windows x64 macOS x64
Build / test ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed)

I was going to ask you what I needed to do so that you could generate this. I guess the short answer is that what I've already done is sufficient. Thanks!

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Oct 24, 2020

@rwestberg I was pleasantly surprised to see the Skara bot add this:

Linux x64
Windows x64
macOS x64

Build / test
heavy_check_mark (1/1 passed)
heavy_check_mark (1/1 passed)
heavy_check_mark (1/1 passed)

I was going to ask you what I needed to do so that you could generate this. I guess the short answer is that what I've already done is sufficient. Thanks!

I noticed this in the openjdk/mobile repository as well, and that looks great. In that repository, there seems to be a different configuration, as there is a line for "build" and a line for "test/tier 1" ( e.g. openjdk/mobile#10 )

I wonder how configurable this is?

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Oct 24, 2020

The jdk workflow is split into a build job and multiple test jobs (since it takes so long they need the parallelism when running the tests). It's more complicated that way, since you have to feed the results of the build job into all of the test jobs, and there is duplication of some of the configuration steps. I don't know that it would buy us much, so I wasn't planning to split the jobs -- at leat not initially.

The build and test are done in separate steps within the job so it's easy to see which one failed. Maybe Robin can comment further as to whether we could do something a bit more clever.

@rwestberg
Copy link
Member

@rwestberg rwestberg commented Oct 26, 2020

I noticed this in the openjdk/mobile repository as well, and that looks great. In that repository, there seems to be a different configuration, as there is a line for "build" and a line for "test/tier 1" ( e.g. openjdk/mobile#10 )

I wonder how configurable this is?

The output is not very configurable at all. :) You can see https://github.com/openjdk/skara/blob/bd4cc7e9a6ff7b04c6dc650a969638d5c1471031/bots/pr/src/main/java/org/openjdk/skara/bots/pr/TestResults.java#L42 for details. But it should support additional tiers at least if any were added in the future.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Oct 26, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Oct 26, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth changed the title WIP: 8254691: Enable GitHub actions for jfx repo 8254691: Enable GitHub actions for jfx repo Oct 26, 2020
@kevinrushforth kevinrushforth marked this pull request as ready for review Oct 26, 2020
@openjdk openjdk bot added the rfr label Oct 26, 2020
@kevinrushforth kevinrushforth requested review from arapte and johanvos Oct 26, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 26, 2020

Webrevs

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Oct 27, 2020

Note to reviewers:

If you want to test this yourself you can pull the PR branch into a branch in your local repo and push it to your personal fork. Something like the following should work for you, assuming that you are in your local repo with origin pointing to your personal fork:

git fetch https://github.com/openjdk/jfx.git pull/338/head:pr-338
git push -u origin pr-338

Then you will be able to see the GitHub action workflow and track its progress by clicking on the "Actions" tab on your personal fork repo page.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Oct 27, 2020

See PR #339 for an example (using an intentionally injected failure) of what a failed build or test will look like.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Oct 28, 2020

I merged master into my local branch so it would run the tests again on an up-to-date repo.

@arapte
arapte approved these changes Oct 29, 2020
Copy link
Member

@arapte arapte left a comment

Looks good to me. Verified a dummy PR with two commits.
Actions executed correctly on both commits. Also verified that test failure is reported correctly and failed test can be found in log under Actions section.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 30, 2020

Mailing list message from Eric Bresie on openjfx-dev:

So this doesn?t show lower level test just basically a build successfully pass/fail on each platform? Or would it show any low level tests if they failed?

Eric Bresie
Ebresie at gmail.com (mailto:Ebresie at gmail.com)

# BOOT_JDK_SHA256: "${{ fromJson(needs.prerequisites.outputs.dependencies).LINUX_X64_BOOT_JDK_SHA256 }}"
BOOT_JDK_VERSION: "15"
BOOT_JDK_FILENAME: "openjdk-15_linux-x64_bin.tar.gz"
BOOT_JDK_URL: "https://download.java.net/java/GA/jdk15/779bf45e88a44cbd9ea6621d33e33db1/36/GPL/openjdk-15_linux-x64_bin.tar.gz"

This comment has been minimized.

@johanvos

johanvos Oct 30, 2020
Collaborator

Should this be 15.0.1 ? (same for win/mac)

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Oct 30, 2020

This works fine. I have a minor comment (about using 15.0.1, but since we don't use that in build.properties either, I think it's not an issue), and apart from that, this looks very good.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Oct 31, 2020

I'd rather update the boot JDK to 15.0.1 as a follow-on fix, so that it matches the version defined in build.properties. I filed the following follow-on issues:

JDK-8255714: Switch FX build to use JDK 15.0.1 as boot JDK
JDK-8255712: GitHub actions: read boot JDK version and URL from build.properties
JDK-8255713: GitHub actions: JavaFX build should discover Visual Studio compiler on system

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Oct 31, 2020

So this doesn?t show lower level test just basically a build successfully pass/fail on each platform? Or would it show any low level tests if they failed?

If there is a build or test failure you get a link to the failing tasks and can look at the log file to see the failures (no summary at the present time, though). For example, here are the results from the test build done in PR #339 :

Linux x64 Windows x64 macOS x64
Build / test ✔️ (1/1 passed) (1/1 failed) (1/1 failed)

Failed test tasks

@openjdk
Copy link

@openjdk openjdk bot commented Oct 31, 2020

@kevinrushforth 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:

8254691: Enable GitHub actions for jfx repo

Reviewed-by: arapte, jvos

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

  • 1a11334: 8244297: Provide utility for testing for memory leaks
  • 0376e60: 8255487: Mark SandboxAppTest unstable on Windows
  • 7da928b: 8255497: [TestBug] Controls unit tests - clean up unnecessary prints on STANDARD_OUT

Please see this link for an up-to-date comparison between the source branch of this pull request and 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 label Oct 31, 2020
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Oct 31, 2020

/integrate

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

@openjdk openjdk bot commented Oct 31, 2020

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

  • 1a11334: 8244297: Provide utility for testing for memory leaks
  • 0376e60: 8255487: Mark SandboxAppTest unstable on Windows
  • 7da928b: 8255497: [TestBug] Controls unit tests - clean up unnecessary prints on STANDARD_OUT

Your commit was automatically rebased without conflicts.

Pushed as commit 51c09e5.

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

@kevinrushforth kevinrushforth deleted the kevinrushforth:8254691-github-actions branch Oct 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants