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

mach: Avoid reopening the file for relocation #3101

Merged
merged 1 commit into from Sep 8, 2017

Conversation

Projects
None yet
5 participants
@woodruffw
Member

woodruffw commented Aug 28, 2017

This commit allows the relocation code to perform install name
and dylib ID changes without reopening the file separately.

I still need to delete the old MachO::Tools-calling code and do some other
cleanup, so this is WIP.

Potential fix for the failing tests in #3079.

@woodruffw

This comment has been minimized.

Show comment
Hide comment
@woodruffw
Member

woodruffw commented Aug 28, 2017

cc @mansimarkaur

@@ -24,7 +24,7 @@ def relocate_dynamic_linkage(relocation)
file.ensure_writable do
if file.dylib?
id = dylib_id_for(file).sub(relocation.old_prefix, relocation.new_prefix)
change_dylib_id(id, file)
file.change_dylib_id(id)

This comment has been minimized.

@mansimarkaur

mansimarkaur Aug 28, 2017

Contributor

The change_dylib_id still accepts 2 arguments. Shouldn't this be file.change_dylib_id(id, file)?

@mansimarkaur

mansimarkaur Aug 28, 2017

Contributor

The change_dylib_id still accepts 2 arguments. Shouldn't this be file.change_dylib_id(id, file)?

This comment has been minimized.

@woodruffw

woodruffw Aug 28, 2017

Member

Whoops, that was a mistake. It should only take 1, I'll fix that up now.

@woodruffw

woodruffw Aug 28, 2017

Member

Whoops, that was a mistake. It should only take 1, I'll fix that up now.

mach: Avoid reopening the file for relocation
This commit allows the relocation code to perform install name
and dylib ID changes without reopening the file separately.
@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Aug 29, 2017

Contributor

@ayan-das1234 please follow our code of conduct when interacting with other people here.

Contributor

ilovezfs commented Aug 29, 2017

@ayan-das1234 please follow our code of conduct when interacting with other people here.

@woodruffw woodruffw changed the title from [WIP] mach: Avoid reopening the file for relocation to mach: Avoid reopening the file for relocation Aug 30, 2017

@woodruffw woodruffw removed the in progress label Aug 30, 2017

@woodruffw woodruffw referenced this pull request Sep 5, 2017

Merged

Added tests for os/mac/keg #3079

4 of 5 tasks complete
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 8, 2017

Member

@woodruffw 🚢 when you're happy.

Member

MikeMcQuaid commented Sep 8, 2017

@woodruffw 🚢 when you're happy.

@woodruffw woodruffw merged commit a77a1f9 into Homebrew:master Sep 8, 2017

1 of 3 checks passed

codecov/patch 16.66% of diff hit (target 50.19%)
Details
codecov/project 50.18% (-0.01%) compared to 2a75c6c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@woodruffw woodruffw deleted the woodruffw-forks:macho-use-object branch Sep 8, 2017

scpeters added a commit to scpeters/brew that referenced this pull request Sep 27, 2017

keg_relocate: fix call to change_install_name
During the changes to macho file relocation refactoring
in Homebrew#3101, Homebrew#3138, and Homebrew#3139,
the number of arguments to the mach::change_install_name
function changed from 3 to 2, but
there was still an instance of the function being called
with the wrong number of arguments.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.