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

8256657: Add cross-compiled build for Windows+Arm64 to submit workflow #1379

Closed

Conversation

lewurm
Copy link
Member

@lewurm lewurm commented Nov 23, 2020

This adds the cross-compiled build only, as no Windows+Arm64 machines are available on GitHub Action that we could use to run the tests.

Due to cross-compilation a build JDK is required. Initially I added EA builds to be downloaded from https://jdk.java.net/16/ and used for that, but then I saw how @shipiliv attempted it for the linux cross-compilation builds in #1147. That is using the JDK image produced by the x64 variant. This however add more stress to the "critical path", as now two more jobs depend on the x64 build first.

Let's see how it works out in the long-run. A Windows+AArch64 build takes 40-50min.


Progress

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

Issue

  • JDK-8256657: Add cross-compiled build for Windows+Arm64 to submit workflow

Reviewers

Download

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

This adds the cross-compiled build only, as no Windows+Arm64 machines are available on GitHub Action that we could use to run the tests.

Due to cross-compilation a build JDK is required. Initially I added EA builds to be downloaded from https://jdk.java.net/16/ and used for that, but then I saw how @shipiliv attempted it for the linux cross-compilation builds in openjdk#1147.  That is using the JDK image produced by the x64 variant. This however add more stress to the "critical path", as now two more jobs depend on the x64 build first.

Let's see how it works out in the long-run. A Windows+AArch64 build takes 40-50min.
@bridgekeeper
Copy link

bridgekeeper bot commented Nov 23, 2020

👋 Welcome back burban! 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 Pull request is ready for review label Nov 23, 2020
@openjdk
Copy link

openjdk bot commented Nov 23, 2020

@lewurm 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 23, 2020
@mlbridge
Copy link

mlbridge bot commented Nov 23, 2020

Webrevs

@mlbridge
Copy link

mlbridge bot commented Nov 23, 2020

Mailing list message from Thomas St��fe on build-dev:

Hi Bernhard,

just some drive-by comments.

- 40-50 min builds seem to be excessive for just the hotspot build, do you
know what exactly takes that long? Is this for release and debug each or
both combined?

- Is it worth the cycles to build both release *and* debug? How probable is
it that a non-win-aarch-dev will break one but not the other? I presume
developers on the windows aarch project will have tested the build locally
before pushing.

- fixpath: Having the gh actions download a patch from your personal repos
seems odd

I feel at some point we need to have a talk about the growing number of
platforms in github actions. E.g. which platforms should be mandatory green
before pushing.

I also am unsure about the associated cost for each developer (I know atm
everyone has a free quota, but that is not limitless. If it is eaten up,
strictly speaking one would not be able to push anymore since no submit
tests can be ran until the quota is refilled).

Cheers, Thomas

On Mon, Nov 23, 2020 at 10:41 AM Bernhard Urban-Forster <
burban at openjdk.java.net> wrote:

@magicus
Copy link
Member

magicus commented Nov 23, 2020

I thought windows-aarch64 was not able to build without a patch? Have you fixed that?

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.

I think you need to wait for #1350, and then reconcile this patch with that refactoring.

Comment on lines 1249 to 1250
- build release
- build debug
Copy link
Member

Choose a reason for hiding this comment

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

Other foreign arches build only debug, thus optimizing the workflow time, drop release here?

Comment on lines 1362 to 1367
- name: Apply fixpath.exe Patch
run: |
curl 'https://gist.githubusercontent.com/lewurm/c099a4b5fcd8a182510cbdeebcb41f77/raw/d5badd6ee78911f79d5c3b695c61debfa54d8ced/0001-fixpath-workaround-for-win-aarch64.patch' > p.patch
git apply p.patch
working-directory: jdk
shell: bash
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the mainline repo instead. Yes, the absence of this patch blocks this PR, but getting that patch into the mainline first is the right thing to do.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. We should not have a build action that requires a patch.

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 patch in its current form is not suitable for upstream.

The initial plan was to wait until the "new WINENV" patch lands [1], which would resolve the problem we face with fixpath.exe. But maybe I can come up with a solution for the current situation, given that I've a better understanding of the build system by now.

[1] https://mail.openjdk.java.net/pipermail/build-dev/2020-July/027872.html

@@ -1856,6 +2018,7 @@ jobs:
- linux_s390x_build
- linux_x64_test
- linux_x86_test
- windows_aarch64_build
Copy link
Member

Choose a reason for hiding this comment

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

This should move to other _build needs, for clarity.

@lewurm
Copy link
Member Author

lewurm commented Nov 23, 2020

Thanks for all the comments.

  • 40-50 min builds seem to be excessive for just the hotspot build, do youknow what exactly takes that long? Is this for release and debug each or both combined?

It's for each of them. Installing a specific version of MSVC and creating the devkit take each ~10min. Thinking about it, there is a opportunity to cache the devkit and do the MSVC installer invocation only if no cached devkit is available.

Here is an example run if you want to have a closer look:
https://github.com/lewurm/openjdk/runs/1434118318?check_suite_focus=true

  • Is it worth the cycles to build both release and debug? How probable is
    it that a non-win-aarch-dev will break one but not the other? I presume
    developers on the windows aarch project will have tested the build locally
    before pushing.

Fair, I'll remove one of them (as suggested by Aleksey in another comment, I'll keep the debug one).

@erikj79
Copy link
Member

erikj79 commented Nov 30, 2020

Generating a devkit every time we build seems like a pretty big waste of time. The OpenJDK build is expected to work without devkits, using an installed Visual Studio. Is this not possible with the Windows aarch64 build? If not, then that really should be fixed.

@magicus
Copy link
Member

magicus commented Dec 3, 2020

FWIW, here is a patch on top of my winenv rewrite that adds windows-aarch64 support on Github Actions:
https://github.com/magicus/openjdk-sandbox/compare/winenv-rewrite...magicus:winenv-aarch64-gha?expand=1

I've used it while developing the winenv rewrite, but pulled it out of the final integration so as to not mix in the question of enabling more build platforms on GHA in the discussion of the rewrite.

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.

This looks good now. It adds ~15 minutes of build time (and unfortunately ~10 minutes to download prereqs), which is quite acceptable.

@openjdk
Copy link

openjdk bot commented Dec 7, 2020

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

8256657: Add cross-compiled build for Windows+Arm64 to submit workflow

Reviewed-by: shade, 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 2 new commits pushed to the master branch:

  • fae7961: 8257884: Re-enable sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java as automatic test
  • 79f1dfb: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@shipilev, @magicus) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 7, 2020
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.

Minor nits.

.github/workflows/submit.yml Outdated Show resolved Hide resolved
.github/workflows/submit.yml Outdated Show resolved Hide resolved
.github/workflows/submit.yml Outdated Show resolved Hide resolved
@shipilev
Copy link
Member

shipilev commented Dec 8, 2020

Also merge from master to get the clean workflow run everywhere?

@lewurm
Copy link
Member Author

lewurm commented Dec 9, 2020

Thank you @shipilev for your comments, I've updated the PR.

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.

Thanks, updates look fine to me.

@lewurm
Copy link
Member Author

lewurm commented Dec 9, 2020

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 9, 2020
@openjdk
Copy link

openjdk bot commented Dec 9, 2020

@lewurm
Your change (at version c07f5d7) is now ready to be sponsored by a Committer.

@magicus
Copy link
Member

magicus commented Dec 9, 2020

/sponsor

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

openjdk bot commented Dec 9, 2020

@magicus @lewurm Since your change was applied there have been 6 commits pushed to the master branch:

  • 616b1f1: 8257516: define test group for manual tests
  • 5bdce9b: 8257639: Update usage of "type" terminology in java.lang.Enum & java.lang.Record
  • b4615c6: 8256580: Fault in new grid display
  • df55ecd: 8257794: Zero: assert(istate->_stack_limit == istate->_thread->last_Java_sp() + 1) failed: wrong on Linux/x86_32
  • fae7961: 8257884: Re-enable sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java as automatic test
  • 79f1dfb: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException

Your commit was automatically rebased without conflicts.

Pushed as commit d3dddb6.

💡 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
4 participants