-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Make CFG_LIBDIR configurable. Fixes #5223 #11045
Conversation
err "libdir must begin with the prefix. Use --prefix to set it accordingly." | ||
fi | ||
|
||
valopt libdir-relative "${CFG_LIBDIR:${#CFG_PREFIX}+1}" "install libraries (relative to prefix" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite recognize this syntax, but why not make the default of this value $CFG_LIBDIR_RELATIVE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${#CFG_PREFIX}
counts the number of characters and the ${CFG_LIBDIR:n}
syntax creates a substring starting at n
. I used this syntax so that someone can set --libdir
only without having to change --libdir-relative
. The reason is that --libdir
is a standard command option for configure AFAIK and packaging people might use it. So using
./configure --prefix=/opt/foo --libdir=/opt/foo/lib64
will be the same as
./configure --prefix=/opt/foo --libdir=/opt/foo/lib64 --libdir-relative=lib64
Discussed with @alexcrichton on IRC: New version which doesn't make CFG_LIBDIR_RELATIVE configurable (since it isn't needed) and also removes the restriction that CFG_LIBDIR must begin with CFG_PREFIX. |
LIBDIR_RELATIVE=bin | ||
fi | ||
|
||
valopt libdir "${CFG_PREFIX}/${LIBDIR_RELATIVE}" "install libraries" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't appear to be any sort of validation that the provided path for this on the command line is an absolute path? I would imagine that havoc would ensue if this case happened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting a relative path should work fine. Imagine you have rust checked out
at /usr/rust and set libdir to ../lib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ... I just gave it some more thought and think that I might be wrong: At some places rustc/rustpkg might use the absolute path and this would then result in strange results since it would then depend on the current working dir to find the correct files.
Anyway I don't think a check here should be needed, because other autotools software doesn't check either AFAIK. Also there aren't any checks for CFG_PREFIX either and setting this one to a relative path would mess up even more.
New commit, which uses LIBDIR_RELATIVE/rustc//lib as you suggested. I haven't tested it enough though (builds take forever on Windows and it's complicated because the snapshot doesn't work now). |
@brson, can you take a look at this as well? This all looks good to me, just want to make sure that I'm not the only person that looks at this. |
To build on Windows run the following after
|
Looks like this just needs a rebase now that #11118 is landed. |
Yes and also fix stage0 on Windows. I'm working on it :) |
New commit. I've also removed trailing whitespace in configure, my editor settings always screwed up the diffs. Btw: After the next snapshot also remove
in |
Use depinfo to discover UI test dependencies changelog: none context: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Building.20test.20dependencies This restores [the old `EXTERN_FLAGS` method](https://github.com/rust-lang/rust-clippy/blob/4cf5bdc60c7ccec3b5f395ee393615c224e28555/tests/compile-test.rs#L67-L75) of passing `--extern` flags for building UI tests with minor changes - Unused deps were removed - It's now a `Vec` of args instead of a command string - It uses a `BTreeMap` so the extern flags are in alphabetical order and deterministic I don't know if the `HOST_LIBS` part is still required, but I figured it best to leave it in for now. If the change is accepted we can take a look if it's needed in `rust-lang/rust` after the next sync This isn't as pleasant as having a `Cargo.toml`, though there is something satisfying about knowing the dependencies are already built and not needing to invoke `cargo` r? `@flip1995`
See #5223 (comment)