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.

Sign up for GitHub

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

Look for crates in LIBDIR, provided by --libdir option #16552

Closed
wants to merge 1 commit into from
Closed

Look for crates in LIBDIR, provided by --libdir option #16552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,9 +550,19 @@ CFG_LIBDIR_RELATIVE=lib
if [ "$CFG_OSTYPE" = "pc-windows-gnu" ]
then
CFG_LIBDIR_RELATIVE=bin
fi
CFG_LIBDIR="${CFG_PREFIX}/${CFG_LIBDIR_RELATIVE}"
else
valopt libdir "${CFG_PREFIX}/${CFG_LIBDIR_RELATIVE}" "install libraries (ignored on windows platform)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not ignore options based on the detected OSTYPE. What's keeping us from calculating the relative directory for windows builds the same way it's calculated for other builds, and just supply a different default CFG_LIBDIR_RELATIVE?

Copy link
Member

Choose a reason for hiding this comment

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

See this comment for some more info, but the gist is that our windows installation process wants dlls in the same dirs as the bins. In general I suspect that windows users rarely want this level of configuration anyway, so it's probably not too bad.


case "$CFG_LIBDIR" in
"$CFG_PREFIX"/*) CAT_INC=2;;
"$CFG_PREFIX"*) CAT_INC=1;;
*)
err "libdir must begin with the prefix. Use --prefix to set it accordingly.";;
esac

valopt libdir "${CFG_PREFIX}/${CFG_LIBDIR_RELATIVE}" "install libraries"
CFG_LIBDIR_RELATIVE=`echo ${CFG_LIBDIR} | cut -c$((${#CFG_PREFIX}+${CAT_INC}))-`
fi

if [ $HELP -eq 1 ]
then
Expand Down Expand Up @@ -989,6 +999,15 @@ for h in $CFG_HOST
do
for t in $CFG_TARGET
do
# host lib dir stage0
make_dir $h/stage0/lib

# target bin dir stage0
make_dir $h/stage0/lib/rustlib/$t/bin

# target lib dir stage0
make_dir $h/stage0/lib/rustlib/$t/lib
Copy link
Member

Choose a reason for hiding this comment

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

The stage0 windows binaries depend on the relative libdir being bin, so this may not end up working on windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove --libdir support for windows. They probably do not need this, as anyway they have problems with LD_LIBRARY_PATH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This make_dir is not relevant for windows as it only create directories, so it can stay as is.


for i in 0 1 2 3
do
# host bin dir
Expand Down
8 changes: 8 additions & 0 deletions mk/main.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,15 @@ define SREQ
# Destinations of artifacts for the host compiler
HROOT$(1)_H_$(3) = $(3)/stage$(1)
HBIN$(1)_H_$(3) = $$(HROOT$(1)_H_$(3))/bin
ifeq ($$(CFG_WINDOWSY_$(3)),1)
HLIB$(1)_H_$(3) = $$(HROOT$(1)_H_$(3))/$$(CFG_LIBDIR_RELATIVE)
else
ifeq ($(1),0)
HLIB$(1)_H_$(3) = $$(HROOT$(1)_H_$(3))/lib
else
HLIB$(1)_H_$(3) = $$(HROOT$(1)_H_$(3))/$$(CFG_LIBDIR_RELATIVE)
endif
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified by using:

ifeq ($$(CFG_WINDOWSY_$(3))-$(1),0-0)
HLIB$(1)_H_$(3) = $$(HROOT$(1)_H_$(3))/lib
else
HLIB$(1)_H_$(3) = $$(HROOT$(1)_H_$(3))/$$(CFG_LIBDIR_RELATIVE)
endif


# Destinations of artifacts for target architectures
TROOT$(1)_T_$(2)_H_$(3) = $$(HLIB$(1)_H_$(3))/rustlib/$(2)
Expand Down
15 changes: 11 additions & 4 deletions src/etc/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,16 @@ fi
flag uninstall "only uninstall from the installation prefix"
opt verify 1 "verify that the installed binaries run correctly"
valopt prefix "/usr/local" "set installation prefix"
# NB This isn't quite the same definition as in `configure`.
# just using 'lib' instead of CFG_LIBDIR_RELATIVE
# NB This is exactly the same definition as in `configure`.
valopt libdir "${CFG_PREFIX}/${CFG_LIBDIR_RELATIVE}" "install libraries"
case "$CFG_LIBDIR" in
"$CFG_PREFIX"/*) CAT_INC=2;;
"$CFG_PREFIX"*) CAT_INC=1;;
*)
err "libdir must begin with the prefix. Use --prefix to set it accordingly.";;
esac
CFG_LIBDIR_RELATIVE=`echo ${CFG_LIBDIR} | cut -c$((${#CFG_PREFIX}+${CAT_INC}))-`
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that rust will work by default if you pass --libdir (even with the changes here), so we may just want to completely remove the option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With these my changes it works. It was the aim of this changeset. )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course you need to add your new libdir to LD_LIBRARY_PATH, but if one provides --libdir, he usually does this.

Copy link
Member

Choose a reason for hiding this comment

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

The problem arises when rust is configured with one --libdir and then installed with a different --libdir, have you tested that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not be done this way. libdir from configure should be used during installation. Otherwise it will be for sure broken. install.sh parameters are passed by make, not by user. and make takes it from configuration.

I see no reason for user try to somehow use different libdirs in configuration and installation. And in my changes libdir is relative, so copying of the whole tree with compiler will work, if you would wondering about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here when I say libdir I mean relative path, not the absolute.

Copy link
Member

Choose a reason for hiding this comment

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

nightly is built (with default libdir, yes), the somebody downloads and unpacks it to the folder he likes. No additional calling of install.sh or whatever happens, just copying.

This is not the only way to install rust, you are expected to use the install.sh script which supports itself a --libdir option, that is where the discrepancy arises.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the current implementation this will not work at all. It means even if user uses the same libdir during configure and install, rust will not work with it different from 'lib'. So this feature is already broken.

And I think it is not sane to change such things during install, not during configure, so we probably should somehow fix it, so user can not change libdir during installation.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the feature is currently broken, and my point is that this commit looks like it's trying to solve it (by closing #11671). This specific commit does not close that issue as this is precisely what it's talking about.

Regardless of whether you feel it's sane or not, it's what the issue is talking about, so this commit cannot close it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I see in original issue (#11671), that rust was configured with --libdir. And installed with the same --libdir. Or am I wrong? Where is it written that --libdir has been changed between configure and install? May be I has not seen it, so if you can, please, point me to the place where it is stated.


valopt mandir "${CFG_PREFIX}/share/man" "install man pages in PATH"

if [ $HELP -eq 1 ]
Expand Down Expand Up @@ -428,9 +435,9 @@ while read p; do
# Decide the destination of the file
FILE_INSTALL_PATH="${CFG_PREFIX}/$p"

if echo "$p" | grep "^lib/" > /dev/null
if echo "$p" | grep "^${CFG_LIBDIR_RELATIVE}/" > /dev/null
then
pp=`echo $p | sed 's/^lib\///'`
pp=`echo $p | sed "s%^${CFG_LIBDIR_RELATIVE}/%%"`
FILE_INSTALL_PATH="${CFG_LIBDIR}/$pp"
fi

Expand Down
15 changes: 10 additions & 5 deletions src/librustc/metadata/filesearch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,16 @@ fn find_libdir(sysroot: &Path) -> String {
// to lib64/lib32. This would be more foolproof by basing the sysroot off
// of the directory where librustc is located, rather than where the rustc
// binary is.

if sysroot.join(primary_libdir_name()).join(rustlibdir()).exists() {
return primary_libdir_name();
} else {
return secondary_libdir_name();
//If --libdir is set during configuration to the value other than
// "lib" (i.e. non-default), this value is used (see issue #16552).

match option_env!("CFG_LIBDIR_RELATIVE") {
Some(libdir) if libdir != "lib" => return libdir.to_string(),
_ => if sysroot.join(primary_libdir_name()).join(rustlibdir()).exists() {
return primary_libdir_name();
} else {
return secondary_libdir_name();
}
Copy link
Member

Choose a reason for hiding this comment

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

This may not be quite right because this environment variable is always set during a normal make which means the fallback case will never actually get hit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note if libdir != "lib", so env variable will be used only if it was set to something not default. And if it was set, user defenetely wants rustc use it, not the fallback.

Copy link
Member

Choose a reason for hiding this comment

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

Ack sorry about that! I was skimming too quickly :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np. )

What I really would ask you to do -- test it on windows if you have no other complains. As I am really not sure whether I did not break something on this system.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just always ensure CFG_LIBDIR_RELATIVE is set an use it? This allows us to move the defaults out of rustc and keep them all in configure.

Copy link
Member

Choose a reason for hiding this comment

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

The reasoning is the same as this comment I believe (I'll reply in a just a bit).

}

#[cfg(target_word_size = "64")]
Expand Down