-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8286429: jpackageapplauncher build fails intermittently in Tier[45] #8618
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
Conversation
|
👋 Welcome back erikj! A progress list of the required criteria for merging this PR into |
|
I'm still unclear how |
|
@erikj79 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: 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 11 new commits pushed to the
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 |
|
I think this looks good. I'm just curious why this started to show up now. There have been no changes in this area lately, have it? Is it just pure (bad) luck that the race did not happen before? Edit: I get it now, this was introduced by JDK-8284675. |
|
@dholmes-ora That option is a bit weirdly named. It is used to enable new generation of man pages from markdown files instead of copying the static troff files. So without that option, this could not have failed. |
|
/integrate |
|
Going to push as commit 65f5067.
Your commit was automatically rebased without conflicts. |
The way LauncherCommon.gmk is currently written, it's only meant to be included from "make/module//Launcher.gmk", or at least only from one single place for each module. This is because the man page generation that happens in LauncherCommon.gmk is module global. For the jdk.jpackage module, LauncherCommon.gmk is now called from two separate sub makefiles, both make/module/jdk.jpackage/Lib.gmk and make/module/jdk.jpackage/Launcher.gmk. These files are called from the top level targets jdk.jpackage-libs and jdk.jpackage-launchers respectively. These top level targets are assumed to be able to run independently of each other, which is normally fine, but now they define the same rules for the same files. This creates a race where one may be deleting files that the other one is creating, causing directories to disappear while files are being written to them. This can fail the build, but also risks silently corrupting the build.
This patch fixes this by adding a conditional around the man page generation, which helps guarantee that man pages are only processed when called from make/module//Launcher.gmk. It's a bit of a hack, but it's building on top of the existing design of piggybacking man page generation on the launchers build.
Also fixing broken whitespace further down in the file (tabs->space).
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8618/head:pull/8618$ git checkout pull/8618Update a local copy of the PR:
$ git checkout pull/8618$ git pull https://git.openjdk.java.net/jdk pull/8618/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8618View PR using the GUI difftool:
$ git pr show -t 8618Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8618.diff