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

Delete last rpath when requested #555

Closed
wants to merge 2 commits into from
Closed

Delete last rpath when requested #555

wants to merge 2 commits into from

Conversation

carlocab
Copy link
Member

Deleting the first rpath is a good default because it mirrors the
behaviour of install_name_tool. However, it's not useful behaviour
when deleting duplicate rpaths, because it changes the order in which
the runtime paths are searched.

We delete duplicate rpaths in brew, so being able to delete the last
instance of the requested rpath instead of the first one would be
useful.

@carlocab
Copy link
Member Author

@woodruffw, I totally forgot that e9eaa75 hadn't made it into a release yet. That would've been useful for avoiding some recent bugs I had to deal with 😅

I'd appreciate if you could cut a release soon-ish with the changes from this PR too! Thanks.

@carlocab
Delete last rpath when requested
d07af37 
Deleting the first rpath is a good default because it mirrors the
behaviour of `install_name_tool`. However, it's not useful behaviour
when deleting duplicate rpaths, because it changes the order in which
the runtime paths are searched.

We delete duplicate rpaths in `brew`, so being able to delete the last
instance of the requested rpath instead of the first one would be
useful.
@carlocab carlocab changed the title Delete last rpath when requested Improve rpath deletion Jul 22, 2023
@carlocab carlocab changed the title Improve rpath deletion Delete last rpath when requested Jul 22, 2023
@carlocab carlocab mentioned this pull request Jul 22, 2023
Merged
7 tasks
woodruffw
woodruffw reviewed Jul 25, 2023
View reviewed changes
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small nit, otherwise LGTM. @carlocab do you think it makes sense to add a testcase for this? I'll leave it to your judgement 🙂

lib/macho/macho_file.rb Outdated Show resolved Hide resolved
@woodruffw
Copy link
Member

I'd appreciate if you could cut a release soon-ish with the changes from this PR too! Thanks.

Sorry about that, apparently I totally forgot to release the other change. I'll do a release for both that and this once we get this in!

@carlocab @woodruffw
Update lib/macho/macho_file.rb
b566332 
Co-authored-by: William Woodruff <[email protected]>
@carlocab
Copy link
Member Author

do you think it makes sense to add a testcase for this?

Added some tests! They're a bit repetitive, sorry. But testing this would be good, I think.

@woodruffw
Copy link
Member

Added some tests! They're a bit repetitive, sorry. But testing this would be good, I think.

All good by me. But I think you might have forgotten to push them 😉

@carlocab
Copy link
Member Author

carlocab commented Jul 25, 2023 •
edited
Loading

I did push! GitHub isn't picking up the changes I made, for some reason... carlocab@b685d3b

@carlocab carlocab closed this Jul 25, 2023
@carlocab
Copy link
Member Author

Reopened in #556.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@woodruffw woodruffw woodruffw left review comments

Assignees
No one assigned
Labels
outdated
Projects
None yet
Milestone
No milestone
Development

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