Skip to content

Fix installing into --libdir other than .../lib #22831

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

Closed
wants to merge 1 commit into from

Conversation

anguslees
Copy link
Contributor

Currently, --libdir path rewriting happens twice:

  1. 'prepare' make target places files into file tree and builds tarball
  2. rust-installer copies from tarball paths into final filesystem

Both these steps are attempting to handle --libdir. The installer assumes
lib paths are 'lib/' (the default) and when they aren't you can end up with
duplicated $(LIBDIR_RELATIVE)/$(LIBDIR_RELATIVE)/ paths.

This patch addresses this by making step (1) always install into lib/ and
performing --libdir handling only during step (2).

Currently, --libdir path rewriting happens twice:
1. 'prepare' make target places files into file tree and builds tarball
2. rust-installer copies from tarball paths into final filesystem

*Both* these steps are attempting to handle --libdir.  The installer assumes
lib paths are 'lib/' (the default) and when they aren't you can end up with
duplicated $(LIBDIR_RELATIVE)/$(LIBDIR_RELATIVE)/ paths.

This patch addresses this by making step (1) always install into lib/ and
performing --libdir handling only during step (2).
@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

r? @brson

I haven't touched this in awhile...

@rust-highfive rust-highfive assigned brson and unassigned alexcrichton Feb 27, 2015
@brson
Copy link
Contributor

brson commented Mar 9, 2015

Yes, I should review this!

@anguslees
Copy link
Contributor Author

@brson (bump)

@steveklabnik
Copy link
Member

Another bump

@brson
Copy link
Contributor

brson commented Jun 15, 2015

I've been willfully ignoring this PR because the ramifications around handling libdir in Rust are more complex than I want to think about. I'm very sorry.

Some observations:

  • This patch retains the 'internal' file hierarchy customized by CFG_LIBDIR_RELATIVE, then undoes that customization when preparing the install image. If we're going in this direction, I'd prefer to keep a consistent file system structure for the build (i.e. always use 'lib' as the directory name), and only frob libdir/mandir, etc. during the final installation (sink it into rust-installer).
  • On windows we use 'bin' for the libdir so that windows' dynamic linker can more easily resolve the dylibs. This patch will break installation on windows by not accounting for that. In a world where we use a consistent directory layout for the build, I might expect rust-installer to automatically install 'lib' files to 'bin' on windows.
  • rustc does not work correctly with unexpected values of 'lib' and 'bin' because it calculates their location from the relative path from the rustc binary.

So I'd say the way I'd want to move forward on this is:

  • Teach rust-installer to automatically install the lib folder to bin on windows, possibly only when configured a certain way since this is weird rust-specific behavior and rust-installer is nominally a generic installer.
  • Remove CFG_LIBDIR_RELATIVE from the makefiles completely so all lib builds go in lib/.

@brson
Copy link
Contributor

brson commented Jun 15, 2015

Ah, hm. The reason this may leave the custom heirarchy around for the build is probably because we use --libdir to statically alter rustcs sysroot calculation (iow if you set--libdir=foorustc looks for libs in../fooinstead of../lib`. I don't like this much because then it produces an installer that is customized to the particular system you are building on.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with comments addressed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants