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

keg_relocate: Don't relocate duplicated Rust dylibs. #2764

Closed

Conversation

woodruffw
Copy link
Member

This is a workaround for rust-lang/rust#39870.

We could make the filename match even more restrictive, but I think that /lib/rustlib/ will probably be exclusive enough.

cc @ilovezfs

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

The code looks fine but I don't know how I feel about the precedent of writing language version specific hacks into our core code. How does this break things for Homebrew users today? Has work started upstream on fixing their issue?

@MikeMcQuaid
Copy link
Member

To add to my previous comment: what are the options for making a change in Homebrew/homebrew-core for this?

@woodruffw
Copy link
Member Author

How does this break things for Homebrew users today?

My understanding is that the rust package is currently half-working, with certain things causing a crash along the lines of rust-lang/rust#39870 due to multiple copies of the same library being loaded and clobbering each other's state.

Has work started upstream on fixing their issue?

They've gotten an issue open at rust-lang/rust#39875, but I don't think any work has been started.

what are the options for making a change in Homebrew/homebrew-core for this?

I think the simplest equivalent solution would be to glob all dylibs under lib/rustlib and re-rewrite them to contain @rpath metavariables in their LC_DYLIB_IDs.

I went with this because it avoids a second rewrite, but it is awfully specific to be included in core. The in-formula solution isn't that much more complex, I'll have it ready in a day or two.

@ilovezfs
Copy link
Contributor

I'm 👎 on basically permanently (I don't think upstream is going to "fix" something that isn't broken) carrying an in-formula "solution" for this since it's unwinding changes that brew just made.

Copy link
Contributor

@ilovezfs ilovezfs left a comment

Choose a reason for hiding this comment

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

I'd rather see effort directed at a general solution in brew for this situation than accepting a hack into homebrew/core that will have little to no prospect of ever being removed.

@woodruffw
Copy link
Member Author

general solution in brew

This PR is pretty specific right now, but I could definitely morph it into a more general solution for excluding files or patterns from relocation. That would avoid making a rust-specific change to brew and undoing our own relocations in postinstall.

@ilovezfs
Copy link
Contributor

Maybe take a look at skip_clean for inspiration?

@woodruffw
Copy link
Member Author

Maybe take a look at skip_clean for inspiration?

Thanks. Yeah, I was thinking something along the lines of skip_relocation "some/path/*.glob".

One potential problem is that we don't need to skip install name relocation in rust's case - only dylib ID relocation. I could split those two up, but you can see how that would get ugly/complicated fast.

@ilovezfs
Copy link
Contributor

One potential problem is that we don't need to skip install name relocation in rust's case

Is it a problem though? Does rust break if we don't do the install name relocation either?

@woodruffw
Copy link
Member Author

Does rust break if we don't do the install name relocation either?

Actually, I'm not sure. I wouldn't expect it to break with the unrelocated paths, we just haven't needed to do it. I'll test this evening.

@ilovezfs
Copy link
Contributor

FYI, I've shipped 1.18.0 (Homebrew/homebrew-core#14375) in its currently "broken" state so no need to worry about that waiting on this or anything like that.

woodruffw added a commit to woodruffw-forks/homebrew-core that referenced this pull request Jun 11, 2017
@woodruffw
Copy link
Member Author

I think we should go ahead and merge Homebrew/homebrew-core#14490, since this is indeed pretty specific to one language/formula.

I'm going to test how the rustlib/ dylibs behave with no install name relocation later today, and I'll make another PR along the lines of skip_relocation "some/path" if it works out.

@woodruffw woodruffw closed this Jun 11, 2017
@woodruffw woodruffw deleted the dont-relocate-rust-dups branch June 11, 2017 19:47
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Jun 11, 2017
ilovezfs pushed a commit to Homebrew/homebrew-core that referenced this pull request Jun 12, 2017
This is the less invasive version of Homebrew/brew#2764.

Closes #14490.

Signed-off-by: ilovezfs <ilovezfs@icloud.com>
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants