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.

Sign up for GitHub

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

Jump to bottom

extend/os/mac/keg_relocate: fix relocation of duplicate RPATHs #11392

Merged
merged 1 commit into from
May 16, 2021
Merged

extend/os/mac/keg_relocate: fix relocation of duplicate RPATHs #11392

merged 1 commit into from
May 16, 2021

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented May 16, 2021 •
edited
Loading

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Homebrew/homebrew-core#77263 revealed two bugs. One in ruby-macho,
which turns out to be unable to delete duplicated RPATHs. This was
fixed with ruby-macho 2.5.1.

The second, which this PR fixes, is in our handling of duplicate
RPATHs. Since we iterate over each RPATH, attempting to relocate the
first duplicate fails since it will no longer exist after having
relocated the original.

Unblocks Homebrew/homebrew-core#77263.

@carlocab carlocab added the critical Critical change which should be shipped as soon as possible. label May 16, 2021
BrewTestBot
BrewTestBot previously approved these changes May 16, 2021
View reviewed changes
BrewTestBot
BrewTestBot previously approved these changes May 16, 2021
View reviewed changes
@carlocab
Copy link
Member Author

carlocab commented May 16, 2021 •
edited
Loading

One potential risk here is that this will change brew's treatment of duplicate install names if having them is even possible (I'm not sure it is).

I'll keep an eye on problems arising from this. If there are any, one way to fix it is to split each_linkage_for into two separate methods (as they were previously): each_install_name_for and each_rpath_for.

@carlocab carlocab mentioned this pull request May 16, 2021
Closed
@carlocab
extend/os/mac/keg_relocate: fix relocation of duplicate RPATHs
e8b5eb7 
Homebrew/homebrew-core#77623 revealed two bugs. One in `ruby-macho`,
which turns out to be unable to delete duplicated `RPATH`s. This was
fixed with `ruby-macho` 2.5.1.

The second, which this commit fixes, is in our handling of duplicate
`RPATH`s. Since we iterate over each `RPATH`, attempting to relocate the
first duplicate fails since it will no longer exist after having
relocated the original.
@carlocab carlocab enabled auto-merge May 16, 2021 23:43
@carlocab carlocab added critical Critical change which should be shipped as soon as possible. and removed critical Critical change which should be shipped as soon as possible. labels May 16, 2021
@carlocab carlocab requested a review from BrewTestBot May 16, 2021 23:45
@carlocab carlocab removed the critical Critical change which should be shipped as soon as possible. label May 16, 2021
@BrewTestBot
Copy link
Member

Review period will end on 2021-05-18 at 00:00:00 UTC.

@BrewTestBot BrewTestBot dismissed their stale review May 16, 2021 23:46

Review period has not ended yet.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label May 16, 2021
@carlocab carlocab added the critical Critical change which should be shipped as soon as possible. label May 16, 2021
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label May 16, 2021
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@carlocab carlocab merged commit 1b1990d into Homebrew:master May 16, 2021
@carlocab carlocab deleted the duplicate-rpaths branch May 16, 2021 23:47
carlocab added a commit to carlocab/ruby-macho that referenced this pull request May 18, 2021
@carlocab
#rpaths should uniq-ify output
b55064d 
Since `delete_rpath` differs from the behaviour of `otool` in that
duplicate rpaths are deleted in a single invocation, we should be
consistent with that and not report duplicates even when they exist.

Otherwise, this can break workflows that iterate through a macho-o
file's rpaths and does something with each of them. I currently have a
workaround for this in brew [1], but I realised this should actually
probably be fixed here.

[1] Homebrew/brew#11392
@carlocab carlocab mentioned this pull request May 18, 2021
Merged
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 16, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BrewTestBot BrewTestBot BrewTestBot approved these changes

Assignees
No one assigned
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@carlocab @BrewTestBot