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

Ignore duplicate rpaths in #change_rpath #438

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

carlocab
Copy link
Member

This causes a deviation in behaviour from install_name_tool, which
will happily change an existing rpath to one that already exists.

Closes #436.

@carlocab
Copy link
Member Author

Even the existing test that I've just deleted seems to differ from install_name_tool's behaviour:

❯ otool -l libdupes.dylib | rg -A2 LC_RPATH
          cmd LC_RPATH
      cmdsize 24
         path dupe (offset 12)
❯ install_name_tool -rpath dupe dupe libdupes.dylib
❯ echo $?
0

This causes a deviation in behaviour from `install_name_tool`, which
will happily change an existing rpath to one that already exists.

Closes Homebrew#436.
@woodruffw
Copy link
Member

Hmm, I wonder if install_name_tool's behavior changed beneath us. It wouldn't be the first time...

@carlocab
Copy link
Member Author

I was wondering that too! But I don't have access to an old Mac to check.

@woodruffw
Copy link
Member

Neither do I anymore, unfortunately. I remember we did quite a bit of behavioral testing against install_name_tool during the initial development here, so it wouldn't surprise me if things have changed. But we'll continue our policy of roughly following those changes 🙂

@github-actions
Copy link

github-actions bot commented Feb 9, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the Stale label Feb 9, 2022
@woodruffw woodruffw removed the Stale label Feb 9, 2022
@github-actions
Copy link

github-actions bot commented Mar 3, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the Stale label Mar 3, 2022
@carlocab
Copy link
Member Author

carlocab commented Mar 3, 2022

Have you had the chance to think about how you want to handle this, @woodruffw?

@github-actions github-actions bot removed the Stale label Mar 3, 2022
@woodruffw
Copy link
Member

Sorry for the delay!

And yeah: I think I agree that this change is for the best.

@woodruffw woodruffw merged commit e9eaa75 into Homebrew:master Mar 3, 2022
@carlocab carlocab deleted the change-dupes branch March 3, 2022 03:10
@carlocab
Copy link
Member Author

carlocab commented Mar 3, 2022

No apologies necessary, and thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2022
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.

change_rpath differs in behaviour from install_name_tool when handling duplicates
2 participants