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 #3

Closed
wants to merge 22 commits into from

Conversation

gnu-andrew
Copy link
Member

@gnu-andrew gnu-andrew commented Mar 5, 2022

Initial attempt to backport openjdk/jdk11u-dev@1faefed

Changes so far:

  • make/autoconf/version-numbers -> common/autoconf/version-numbers
  • Fix version variables (JDK_MAJOR_VERSION, JDK_MINOR_VERSION and JDK_MICRO_VERSION in 8u)
  • Remove boot JDK for Linux (provided by build environment Ubuntu 20.04 in openjdk-8-jdk)
  • Replace --with-version-opt with --with-user-release-suffix
  • Drop --enable-jtreg-failure-handler (unsupported)
  • Move make targets to make invocation (--with-default-make-target not supported)
  • Drop --with-msvc-toolset-version
  • Disable tests as we don't currently have the bundles required to support them

Progress

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

Issues

  • JDK-8253424: Add support for running pre-submit testing using GitHub Actions
  • JDK-8253865: Pre-submit testing using GitHub Actions does not detect failures reliably
  • JDK-8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
  • JDK-8254173: Add Zero, Minimal hotspot targets to submit workflow
  • JDK-8254175: Build no-pch configuration in debug mode for submit checks
  • JDK-8254282: Add Linux x86_32 builds to submit workflow
  • JDK-8255373: Submit workflow artifact name is always "test-results_.zip"
  • JDK-8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch
  • JDK-8256127: Add cross-compiled foreign architectures builds to submit workflow
  • JDK-8256277: Github Action build on macOS should define OS and Xcode versions
  • JDK-8256354: Github Action build on Windows should define OS and MSVC versions
  • JDK-8256414: add optimized build to submit workflow
  • JDK-8256393: Github Actions build on Linux should define OS and GCC versions
  • JDK-8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing
  • JDK-8257056: Submit workflow should apt-get update to avoid package installation errors
  • JDK-8259924: GitHub actions fail on Linux x86_32 with "Could not configure libc6:i386"
  • JDK-8260460: GitHub actions still fail on Linux x86_32 with "Could not configure libc6:i386"
  • JDK-8263667: Avoid running GitHub actions on branches named pr/*
  • JDK-8255305: Add Linux x86_32 tier1 to submit workflow
  • JDK-8255352: Archive important test outputs in submit workflow
  • JDK-8282225: GHA: Allow one concurrent run per PR only

Reviewers

Contributors

  • Alex Kasko <akasko@openjdk.org>
  • Zdeněk Žamberský <zzambers@redhat.com>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk8u-dev pull/3/head:pull/3
$ git checkout pull/3

Update a local copy of the PR:
$ git checkout pull/3
$ git pull https://git.openjdk.java.net/jdk8u-dev pull/3/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3

View PR using the GUI difftool:
$ git pr show -t 3

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk8u-dev/pull/3.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 5, 2022

👋 Welcome back andrew! 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 Backport 1faefed218051c324bdb5c7c10369050d6c9dd44 8253424: Add support for running pre-submit testing using GitHub Actions Mar 5, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Mar 5, 2022

This backport pull request has now been updated with issues from the original commit.

@openjdk openjdk bot added backport rfr labels Mar 5, 2022
@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Mar 7, 2022

@gnu-andrew Could you mark this PR as draft until it's ready for review, please? This avoids many emails sent to the list unnecessarily.

@gnu-andrew gnu-andrew marked this pull request as draft Mar 7, 2022
@gnu-andrew
Copy link
Member Author

@gnu-andrew gnu-andrew commented Mar 7, 2022

@gnu-andrew Could you mark this PR as draft until it's ready for review, please? This avoids many emails sent to the list unnecessarily.

Done. Sorry, I didn't realise it wasn't spamming anyone.

I've done as much as I can here. I'm going to need to enlist someone to look at the Windows build. The additional builds & tests will need backports of bundle support.

@openjdk openjdk bot removed the rfr label Mar 7, 2022
@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Mar 8, 2022

I've done as much as I can here. I'm going to need to enlist someone to look at the Windows build. The additional builds & tests will need backports of bundle support.

Maybe @akashche or @stooke can help with the Windows GHA builds?

@akashche
Copy link
Contributor

@akashche akashche commented Mar 8, 2022

@jerboaa , I don't have experience with GitHub Actions, will look into them and report back.

@gdams
Copy link

@gdams gdams commented Mar 9, 2022

Maybe @akashche or @stooke can help with the Windows GHA builds?

I've played around with trying to get JDK8u to compile on Windows in GH actions before, I think it mainly boils down to the version of Visual Studio on the executors. At Eclipse Adoptium we use Visual Studio 2013 but there is no easy way to install that onto the environment anymore (we used to use chocolatey but that package broke a while back). Happy to provide any help if required

@gnu-andrew
Copy link
Member Author

@gnu-andrew gnu-andrew commented Mar 9, 2022

Maybe @akashche or @stooke can help with the Windows GHA builds?

I've played around with trying to get JDK8u to compile on Windows in GH actions before, I think it mainly boils down to the version of Visual Studio on the executors. At Eclipse Adoptium we use Visual Studio 2013 but there is no easy way to install that onto the environment anymore (we used to use chocolatey but that package broke a while back). Happy to provide any help if required

Any help is very much welcome. I've never built on Windows before, so I have no idea of the requirements and issues there. What we have at present is pretty much what is in 11u-dev, other than I removed the --with-msvc-toolset-version option which 8u doesn't support. Maybe we have to either look into getting 8u to support newer versions of Windows or forego automated Windows testing. The former is risky, but preferable to the latter.

@gnu-andrew
Copy link
Member Author

@gnu-andrew gnu-andrew commented Mar 9, 2022

@jerboaa , I don't have experience with GitHub Actions, will look into them and report back.

It's largely just a collection of build scripts with interdependencies. The main part for you to look at is under windows_x64_build in https://github.com/openjdk/jdk8u-dev/pull/3/files#diff-d9aafa5b5fb68a30f87fd41d7f84a7087fd33b0b07e406fd1ba4b3e66a6b9683R756 and the configure failure in https://github.com/gnu-andrew/jdk8u-dev/runs/5442291679?check_suite_focus=true It may be that this is already familiar to you from your own builds.

JTREG_VERSION=5.1
JTREG_BUILD=b01

WINDOWS_X64_BOOT_JDK_FILENAME=openjdk-11_windows-x64_bin.zip
Copy link

@gdams gdams Mar 9, 2022

Choose a reason for hiding this comment

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

Suggested change
WINDOWS_X64_BOOT_JDK_FILENAME=openjdk-11_windows-x64_bin.zip
WINDOWS_X64_BOOT_JDK_FILENAME=openjdk-8_windows-x64_bin.zip

Copy link
Member Author

@gnu-andrew gnu-andrew Mar 10, 2022

Choose a reason for hiding this comment

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

Thanks for catching this. It's only the name for the downloaded file, but it's better that the label matches what's in the box :)

WINDOWS_X64_BOOT_JDK_URL=https://github.com/adoptium/temurin8-binaries/releases/download/jdk8u322-b06/OpenJDK8U-jdk_x64_windows_hotspot_8u322b06.zip
WINDOWS_X64_BOOT_JDK_SHA256=c9e06afb5df850e90a4da9da31c2edf10bd6da9962c4b253e91b41237f8fb2fb

MACOS_X64_BOOT_JDK_FILENAME=openjdk-11_osx-x64_bin.tar.gz
Copy link

@gdams gdams Mar 9, 2022

Choose a reason for hiding this comment

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

Suggested change
MACOS_X64_BOOT_JDK_FILENAME=openjdk-11_osx-x64_bin.tar.gz
MACOS_X64_BOOT_JDK_FILENAME=openjdk-8_osx-x64_bin.tar.gz

@zzambers
Copy link
Contributor

@zzambers zzambers commented Mar 9, 2022

I was doing some experiments with windows (32-bit) builds of jdk8 in github actions in the past using vs2010express. (Visual Studio 2010 is mentioned for windows in README-builds.html) When I tried my old code, and have found, that it no longer works (simple steps to install vs2010express no longer work, caused by dead MS links [1]). However, after some experimentation, I was able to install vs2010express from iso (from webarchive) and now it works again.

My basic (cleaned up) github workflow which does 32-bit build of openjdk8 using vs2010express can be found here. [2]
(feel free to use any of that code)

[1] https://github.community/t/how-to-use-vc100-for-msbuild/16623/2
[2] https://github.com/zzambers/test-jdk-builder/blob/build-ojdk8-win/.github/workflows/build-workflow.yaml

@zzambers
Copy link
Contributor

@zzambers zzambers commented Mar 10, 2022

But there is one problem, which needs to be workarounded in my setup (see that ugly sed [3] in my workflow), to get through configure, as otherwise it dies with this error (seems like problem with path conversion somewhere, but I don't remember exactly, as this is from my old code):

...
checking if fixpath can be created... no
Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 16.00.30319.01 for 80x86
Copyright (C) Microsoft Corporation.  All rights reserved.

fixpath.c
Microsoft (R) Incremental Linker Version 10.00.30319.01
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:D:A:/test-jdk-builder/test-jdk-builder/jdk8u-dev/build/windows-x86-normal-server-fastdebug/fixpath.exe 
fixpath.obj 
LINK : fatal error LNK1104: cannot open file 'D:A:/test-jdk-builder/test-jdk-builder/jdk8u-dev/build/windows-x86-normal-server-fastdebug/fixpath.exe'
configure: error: Could not create /D/a/test-jdk-builder/test-jdk-builder/jdk8u-dev/build/windows-x86-normal-server-fastdebug/fixpath.exe

EDIT:
Problem is that parameter conversion, performed by msys2 on parameters, is wrong in one particular case causing configure script failure. I have found better workaround for this issue without actually modifying jdk code. But it is likely, that this issue will not affect github workflow of this project (jdk8u-dev) as it uses cygwin rather than msys2.

[3] https://github.com/zzambers/test-jdk-builder/blob/ff5f0c9662dc6829c1b267cecfdfdc74ed0af3a1/.github/workflows/build-workflow.yaml#L58
[4] zzambers/test-jdk-builder@e26f3bd

@shipilev
Copy link
Contributor

@shipilev shipilev commented Mar 10, 2022

@gnu-andrew -- please consider adding JDK-8282225 to this batch as well.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 19, 2022

@gnu-andrew
Adding additional issue to issue list: 8282225: GHA: Allow one concurrent run per PR only.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 19, 2022

@gnu-andrew
Removing additional issue from issue list: 8259679.

@gnu-andrew gnu-andrew marked this pull request as ready for review Apr 3, 2022
@openjdk openjdk bot added the rfr label Apr 3, 2022
jerboaa
jerboaa approved these changes Apr 4, 2022
Copy link
Contributor

@jerboaa jerboaa left a comment

This seems good enough for an initial integration. Better to have some (build) tests prior integration than nothing :)

One question: Do we really need file .github/workflows/freetype.vcxproj? If not, please remove.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 4, 2022

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

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

8253424: Add support for running pre-submit testing using GitHub Actions
8253865: Pre-submit testing using GitHub Actions does not detect failures reliably
8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
8254173: Add Zero, Minimal hotspot targets to submit workflow
8254175: Build no-pch configuration in debug mode for submit checks
8254282: Add Linux x86_32 builds to submit workflow
8255373: Submit workflow artifact name is always "test-results_.zip"
8255895: Submit workflow artifacts miss hs_errs/replays due to ZIP include mismatch
8256127: Add cross-compiled foreign architectures builds to submit workflow
8256277: Github Action build on macOS should define OS and Xcode versions
8256354: Github Action build on Windows should define OS and MSVC versions
8256414: add optimized build to submit workflow
8256393: Github Actions build on Linux should define OS and GCC versions
8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing
8257056: Submit workflow should apt-get update to avoid package installation errors
8259924: GitHub actions fail on Linux x86_32 with "Could not configure libc6:i386"
8260460: GitHub actions still fail on Linux x86_32 with "Could not configure libc6:i386"
8263667: Avoid running GitHub actions on branches named pr/*
8255305: Add Linux x86_32 tier1 to submit workflow
8255352: Archive important test outputs in submit workflow
8282225: GHA: Allow one concurrent run per PR only

Co-authored-by: Alex Kasko <akasko@openjdk.org>
Co-authored-by: Zdeněk Žamberský <zzambers@redhat.com>
Reviewed-by: sgehwolf

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 13 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 label Apr 4, 2022
@gnu-andrew
Copy link
Member Author

@gnu-andrew gnu-andrew commented Apr 4, 2022

This seems good enough for an initial integration. Better to have some (build) tests prior integration than nothing :)

One question: Do we really need file .github/workflows/freetype.vcxproj? If not, please remove.

It was provided by Alex for the Windows build: #3 (comment)

I'm no expert as to why this is needed. I did notice there were some FreeType build changes that could possibly be backported. I'd rather we did this in a follow-on PR though, so we can get the basics in. It's already been a month, and personally, I'm not going to have time to look at anything for the next few weeks, due to the security update.

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Apr 4, 2022

This seems good enough for an initial integration. Better to have some (build) tests prior integration than nothing :)
One question: Do we really need file .github/workflows/freetype.vcxproj? If not, please remove.

It was provided by Alex for the Windows build: #3 (comment)

OK. No expert on this either, so fine with me going in either way :-) It's certainly better than what we have currently.

@akashche
Copy link
Contributor

@akashche akashche commented Apr 4, 2022

This seems good enough for an initial integration. Better to have some (build) tests prior integration than nothing :)
One question: Do we really need file .github/workflows/freetype.vcxproj? If not, please remove.

It was provided by Alex for the Windows build: #3 (comment)

OK. No expert on this either, so fine with me going in either way :-) It's certainly better than what we have currently.

This freetype.vcxproj file is used to replace the default FreeType build file. It has modifications to be buildable with VS2017. It is necessary for --with-freetype-src to work, with this flag FreeType is being built with msbuild (that used this build file) at configure time. We can avoid it by building FreeType by some other means (before running configure) and using --with-freetype-lib and --with-freetype-include instead. Just this --with-freetype-src seems to be "the most vanilla" way to build FreeType for 8u, so I suggest to continue using it.

@gnu-andrew
Copy link
Member Author

@gnu-andrew gnu-andrew commented Apr 5, 2022

This seems good enough for an initial integration. Better to have some (build) tests prior integration than nothing :)
One question: Do we really need file .github/workflows/freetype.vcxproj? If not, please remove.

It was provided by Alex for the Windows build: #3 (comment)

OK. No expert on this either, so fine with me going in either way :-) It's certainly better than what we have currently.

Thanks. I've flagged the main bug for 8u approval now (I presume we can infer the others from that one).

@gnu-andrew
Copy link
Member Author

@gnu-andrew gnu-andrew commented Apr 5, 2022

This seems good enough for an initial integration. Better to have some (build) tests prior integration than nothing :)
One question: Do we really need file .github/workflows/freetype.vcxproj? If not, please remove.

It was provided by Alex for the Windows build: #3 (comment)

OK. No expert on this either, so fine with me going in either way :-) It's certainly better than what we have currently.

This freetype.vcxproj file is used to replace the default FreeType build file. It has modifications to be buildable with VS2017. It is necessary for --with-freetype-src to work, with this flag FreeType is being built with msbuild (that used this build file) at configure time. We can avoid it by building FreeType by some other means (before running configure) and using --with-freetype-lib and --with-freetype-include instead. Just this --with-freetype-src seems to be "the most vanilla" way to build FreeType for 8u, so I suggest to continue using it.

Thanks for the explanation. That does make things a lot clearer for me.

A couple of questions:

  1. Does this mean the file is not needed for the VS2010 build? I duplicated your changes in that build and it seems to work at present.
  2. Any idea why 11u (which this is backported from) works without this? Does it build FreeType in a different way?

@akashche
Copy link
Contributor

@akashche akashche commented Apr 5, 2022

A couple of questions:

  1. Does this mean the file is not needed for the VS2010 build? I duplicated your changes in that build and it seems to work at present.

Yes, I believe it is not needed for VS2010. I haven't tried it with 32-bit VS2010 myself, but IIRC it worked out of the box with 64-bit VS2013 (freeware version of VS2010 doesn't support 64-bit, setup is possible with WinSDK7.1 toolchain, but it is complicated). Modified file was intended to change only 64-bit config (to make it working with 64-bit VS2017) so 32-bit config remained the same there. Thus Copy FreeType project file step on 32-bit most probably can be removed.

  1. Any idea why 11u (which this is backported from) works without this? Does it build FreeType in a different way?

Yes, in 8u FreeType is not included in-tree, it was always used as an external dependency and for "open" jdk only (Oracle JDK didn't use it). --with-freetype-src flag is relatively recent, it was added in JDK-8057538. In 11u FreeType is included in-tree and is built the same way as other in-tree dep libs.

@gnu-andrew
Copy link
Member Author

@gnu-andrew gnu-andrew commented Apr 6, 2022

I see the bug now has jdk8u-fix-yes.

@gnu-andrew
Copy link
Member Author

@gnu-andrew gnu-andrew commented Apr 6, 2022

A couple of questions:

  1. Does this mean the file is not needed for the VS2010 build? I duplicated your changes in that build and it seems to work at present.

Yes, I believe it is not needed for VS2010. I haven't tried it with 32-bit VS2010 myself, but IIRC it worked out of the box with 64-bit VS2013 (freeware version of VS2010 doesn't support 64-bit, setup is possible with WinSDK7.1 toolchain, but it is complicated). Modified file was intended to change only 64-bit config (to make it working with 64-bit VS2017) so 32-bit config remained the same there. Thus Copy FreeType project file step on 32-bit most probably can be removed.

Thanks. We can maybe try that in a follow-up PR.

  1. Any idea why 11u (which this is backported from) works without this? Does it build FreeType in a different way?

Yes, in 8u FreeType is not included in-tree, it was always used as an external dependency and for "open" jdk only (Oracle JDK didn't use it). --with-freetype-src flag is relatively recent, it was added in JDK-8057538. In 11u FreeType is included in-tree and is built the same way as other in-tree dep libs.

Interesting, thanks. It may be worth considering whether to bring the FreeType sources into 8u as well.

@gnu-andrew
Copy link
Member Author

@gnu-andrew gnu-andrew commented Apr 6, 2022

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Apr 6, 2022

Going to push as commit 10029f7.
Since your change was applied there have been 14 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Apr 6, 2022
@openjdk openjdk bot closed this Apr 6, 2022
@openjdk openjdk bot removed ready rfr labels Apr 6, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Apr 6, 2022

@gnu-andrew Pushed as commit 10029f7.

💡 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
backport integrated
6 participants