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

Fix deletion of duplicate rpaths #357

Merged
merged 2 commits into from May 15, 2021

Conversation

carlocab
Copy link
Member

Deferring field population to the end seems to break rpath deletion of
duplicated rpaths.

Closes #356.


This might not be the ideal solution, but I think duplicated rpaths happen rarely enough for the performance hit to not be that bad.

Also, getting this into Homebrew/brew would unblock Homebrew/homebrew-core#77263.

@carlocab carlocab changed the title Fix deletion of multiple rpaths Fix deletion of duplicate rpaths May 14, 2021
Deferring field population to the end seems to break rpath deletion of
duplicated rpaths.

Closes Homebrew#356.
@carlocab
Copy link
Member Author

carlocab commented May 15, 2021

I've added a test for the deletion of duplicate rpaths. To do so, I've checked in a binary libdupe.dylib into your test fixtures and amended the rpath deletion test to also delete the duplicated /usr/lib rpath:

❯ otool -l test/bin/x86_64/libdupe.dylib | rg -A2 LC_RPATH
          cmd LC_RPATH
      cmdsize 24
         path /usr/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 24
         path /usr/lib (offset 12)

I probably could've amended your Makefile to also generate duplicate rpaths in one of your existing test fixtures, but I was worried about breaking your existing tests.

@woodruffw woodruffw self-assigned this May 15, 2021
@woodruffw woodruffw self-requested a review May 15, 2021 16:36
@woodruffw
Copy link
Member

Thanks for the fix! It's strange that deferring the population breaks deletion of duplicates, so that's something I should take a closer look at. This LGTM and I agree that the performance hit probably isn't too severe, so I'll merge now.

@woodruffw woodruffw merged commit 26a92d3 into Homebrew:master May 15, 2021
@carlocab carlocab deleted the duplicate-rpaths branch May 15, 2021 16:51
@woodruffw
Copy link
Member

Cut with 2.5.1, thanks again!

@carlocab
Copy link
Member Author

Thanks for the fix!

No problem! Thanks for your work on ruby-macho. I wish I had a better fix for you, but a lot of this stuff is way over my head at the moment.

It's strange that deferring the population breaks deletion of duplicates, so that's something I should take a closer look at.

Yup, I agree.

Cut with 2.5.1, thanks again!

Can we get that into Homebrew/brew?

@woodruffw
Copy link
Member

Can we get that into Homebrew/brew?

Yeah; it should be as simple as brew vendor-gems; I think there's a vendor-gems workflow that can be used to run that update directly on the CI. I might not have permissions to do that anymore, though 🙂

@carlocab
Copy link
Member Author

Oh, yes, I was vaguely aware of that. Just never used vendor-gems before. Ruby is still mostly voodoo to me 😄

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rpath deletion fails with duplicate rpaths
2 participants