8342858: Make target mac-jdk-bundle fails on chmod command

[erikj79]

The target mac-jdk-bundle can fail randomly. MacBundles.gmk defines a large number of individual copy rules, which can execute in any order. The "install-file" (our copy) macro on macos includes a check for weird attributes using xattr so that we can remove them. We use the switch -s to make sure that xattr operates on symlinks instead of their targets. However, if we find something, we also run chmod to make sure we have the permissions to remove attributes in the first place, but the chmod command does not have the -h switch to operate on the symlink instead of the target. So if the symlink gets copied before its target, there is a chance that we try to run chmod before the target exists, which will result in a failure. My proposed fix is to add -h to the chmod command so that everything operates on the symlink instead of the target.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8342858: Make target mac-jdk-bundle fails on chmod command (Bug - P4)

Reviewers

• Lutz Schmidt (@RealLucy - Reviewer)
• Magnus Ihse Bursie (@magicus - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21649/head:pull/21649
$ git checkout pull/21649

Update a local copy of the PR:
$ git checkout pull/21649
$ git pull https://git.openjdk.org/jdk.git pull/21649/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21649

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21649.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back erikj! 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]

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

8342858: Make target mac-jdk-bundle fails on chmod command

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

• afb62f7: 8342683: Use non-short forward jump when passing stop()
• 964d8d2: 8340445: [PPC64] Wrong ConditionRegister used in ppc64.ad: flagsRegCR0 cr1
• 7131f05: 8342043: Split Opaque4Node into OpaqueTemplateAssertionPredicateNode and OpaqueNotNullNode
• 37cfaa8: 8338449: ubsan: division by zero in sharedRuntimeTrans.cpp
• a1ef818: 8342825: Fix order of @param tags in module java.desktop
• cdad728: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY
• 018db8c: 8342809: C2 hits "assert(is_If()) failed: invalid node class: Con" during IGVN due to unhandled top
• f1f1537: 8341453: java/awt/a11y/AccessibleJTableTest.java fails in some cases where the test tables are not visible
• 476d0f1: 8339309: unused-variable warnings happen in libfontmanager

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]

@erikj79 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.

[mlbridge]

Webrevs

• 00: Full (1b64396a)

[RealLucy]

What about running chmod only against real files?

	else \
	  $(CP) -fRP '$(call DecodeSpace, $<)' '$(call DecodeSpace, $@)'; \
	  if [ -n "`$(XATTR) -ls '$(call DecodeSpace, $@)'`" ]; then \
            $(CHMOD) -h u+w '$(call DecodeSpace, $@)'; \
	    $(XATTR) -cs '$(call DecodeSpace, $@)'; \
	  fi \
	fi

And maybe add an explaining comment after line 122:

  # The above fixup actions are only necessary if the file is actually copied.
  # When creating just a softlink, we rely on the softlink target being fixed when copied.

[erikj79]

The user reporting this issue actually had an attribute on the softlink itself, which I think we want to remove, so I would rather operate on the softlink. I haven't investigated how the attribute appeared on the softlink, but the purpose of this code is to clear away any unexpected attributes from files in the image.

[RealLucy]

This phenomenon is exploiting all possible facets...
In that case, massaging the softlink is necessary, I agree.

[RealLucy]

LGTM.

[magicus]

This finally explains the odd problem you reported in #20837 (comment). That has been worrying me a bit, if there were some issue with that PR after all, but it turrned out to just be an unlikely race. phew

[erikj79]

This finally explains the odd problem you reported in #20837 (comment). That has been worrying me a bit, if there were some issue with that PR after all, but it turrned out to just be an unlikely race. phew

I'm not sure that error was caused by this issue. That would imply that I had strange file attributes on files in my build directory, and I don't think I do.

[erikj79]

/integrate

[openjdk]

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

• afb62f7: 8342683: Use non-short forward jump when passing stop()
• 964d8d2: 8340445: [PPC64] Wrong ConditionRegister used in ppc64.ad: flagsRegCR0 cr1
• 7131f05: 8342043: Split Opaque4Node into OpaqueTemplateAssertionPredicateNode and OpaqueNotNullNode
• 37cfaa8: 8338449: ubsan: division by zero in sharedRuntimeTrans.cpp
• a1ef818: 8342825: Fix order of @param tags in module java.desktop
• cdad728: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY
• 018db8c: 8342809: C2 hits "assert(is_If()) failed: invalid node class: Con" during IGVN due to unhandled top
• f1f1537: 8341453: java/awt/a11y/AccessibleJTableTest.java fails in some cases where the test tables are not visible
• 476d0f1: 8339309: unused-variable warnings happen in libfontmanager

Your commit was automatically rebased without conflicts.

[openjdk]

@erikj79 Pushed as commit a522d21.

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

[magicus]

I'm not sure that error was caused by this issue. That would imply that I had strange file attributes on files in my build directory, and I don't think I do.

I can't see what else did. I think the strange attributes are injected a bit arbitrarily by Apple. Check all your JDK files, if you dare. ;-)

[erikj79]

I'm not sure that error was caused by this issue. That would imply that I had strange file attributes on files in my build directory, and I don't think I do.

I can't see what else did. I think the strange attributes are injected a bit arbitrarily by Apple. Check all your JDK files, if you dare. ;-)

Oh well, I just did and you were right, I have the provenance attribute on my whole build dir atm.