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 #3139

Merged
merged 1 commit into from Sep 26, 2017

Conversation

Projects
None yet
3 participants
@woodruffw
Member

woodruffw commented Sep 8, 2017

Second time's the charm...

cc @ilovezfs

@woodruffw

This comment has been minimized.

Show comment
Hide comment
@woodruffw

woodruffw Sep 11, 2017

Member

openssl now builds successfully with this patch and the tests are 🍏, so I think this is good to go. I'll merge it tonight unless anybody has new feedback on it.

Member

woodruffw commented Sep 11, 2017

openssl now builds successfully with this patch and the tests are 🍏, so I think this is good to go. I'll merge it tonight unless anybody has new feedback on it.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Sep 12, 2017

Contributor

@woodruffw I think it would be good to add a test here that would have caused a failure for #3101.

Also, in Homebrew/homebrew-core#17861 (comment) @philn raised a good point, I think, about whether those errors should in general be fatal, and why they aren't currently.

Contributor

ilovezfs commented Sep 12, 2017

@woodruffw I think it would be good to add a test here that would have caused a failure for #3101.

Also, in Homebrew/homebrew-core#17861 (comment) @philn raised a good point, I think, about whether those errors should in general be fatal, and why they aren't currently.

@woodruffw

This comment has been minimized.

Show comment
Hide comment
@woodruffw

woodruffw Sep 12, 2017

Member

I think it would be good to add a test here that would have caused a failure for #3101.

Will do!

about whether those errors should in general be fatal, and why they aren't currently.

Hmm. I think the original reason for them being non-fatal was because, in principle, executables with broken linkages can still be run (so long as the broken references are never loaded). That might only be true for lazy-loaded dylibs, however.

I'm 👍 on making install linkage errors fatal by default (with a manual override perhaps?).

Member

woodruffw commented Sep 12, 2017

I think it would be good to add a test here that would have caused a failure for #3101.

Will do!

about whether those errors should in general be fatal, and why they aren't currently.

Hmm. I think the original reason for them being non-fatal was because, in principle, executables with broken linkages can still be run (so long as the broken references are never loaded). That might only be true for lazy-loaded dylibs, however.

I'm 👍 on making install linkage errors fatal by default (with a manual override perhaps?).

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 18, 2017

Member

I'm 👍 on making install linkage errors fatal by default (with a manual override perhaps?).

👍 on this but don't think we need an override (for now, anyway).

Member

MikeMcQuaid commented Sep 18, 2017

I'm 👍 on making install linkage errors fatal by default (with a manual override perhaps?).

👍 on this but don't think we need an override (for now, anyway).

@woodruffw

This comment has been minimized.

Show comment
Hide comment
@woodruffw

woodruffw Sep 18, 2017

Member

👍 on this but don't think we need an override (for now, anyway).

Cool. I'll add it to this PR, along with the tests when I have time.

Member

woodruffw commented Sep 18, 2017

👍 on this but don't think we need an override (for now, anyway).

Cool. I'll add it to this PR, along with the tests when I have time.

@woodruffw woodruffw referenced this pull request Sep 25, 2017

Merged

Added tests for os/mac/keg #3079

4 of 5 tasks complete
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.
@woodruffw

This comment has been minimized.

Show comment
Hide comment
@woodruffw

woodruffw Sep 25, 2017

Member

@ilovezfs I think the failure in #3101 was caused by the change in class, so not something I can easily add to unit tests. Mind if I skip adding tests here, and shore up @mansimarkaur's work in #3079 instead?

Member

woodruffw commented Sep 25, 2017

@ilovezfs I think the failure in #3101 was caused by the change in class, so not something I can easily add to unit tests. Mind if I skip adding tests here, and shore up @mansimarkaur's work in #3079 instead?

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 26, 2017

Member

@woodruffw That works for me! 🚢 when you're confident.

Member

MikeMcQuaid commented Sep 26, 2017

@woodruffw That works for me! 🚢 when you're confident.

@woodruffw woodruffw merged commit 6098998 into Homebrew:master Sep 26, 2017

1 of 3 checks passed

codecov/patch 21.73% of diff hit (target 67.08%)
Details
codecov/project 67.07% (-0.02%) compared to a589303
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 26, 2017

Member

Apologies to @ilovezfs and @woodruffw here: I should not have 👍d this being merged without a regression test that @ilovezfs had specifically requested.

Member

MikeMcQuaid commented Sep 26, 2017

Apologies to @ilovezfs and @woodruffw here: I should not have 👍d this being merged without a regression test that @ilovezfs had specifically requested.

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

@woodruffw woodruffw deleted the woodruffw-forks:macho-use-object branch Jul 3, 2018

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