Look for crates in LIBDIR, provided by --libdir option #16552
Conversation
|
Oooops, it fails. What syntax version of bash/other shell should be used in configure? |
|
configure uses plain sh. |
|
@huonw, thanks, will fix commit appropriately |
|
Still stuff in install to be fixed. |
|
Now it is runnable by plain shell. btw, checkbashisms configure shows other possible problems with configure: |
|
Travis CI fails now because of issues urelated to this commit. |
| return secondary_libdir_name(); | ||
| }, | ||
|
|
||
| Some(libdir) => return libdir.to_string() |
There was a problem hiding this comment.
What was the goal of this change? It doesn't look like CFG_LIBDIR_RELATIVE is put into the environment anywhere at build time here.
There was a problem hiding this comment.
It is put there. In main.mk you have a bunch of exports. I have tested it and it works. )
|
So I would remove support for libdir for Windows as it does not make much sense for this OS and for unix stay with my changes. As with them everything works well. I have tested it. |
|
If you are interested what --libdir can be usefull for, you can have a look at https://github.com/Heather/gentoo-rust/pull/28. tl;dr: this change allows to have different versions of rust installed simultaneously. |
|
So, what is wrong with this PR? It already perfectly works in Gentoo rust installation (https://github.com/Heather/gentoo-rust/tree/master/dev-lang/rust) and I can not see where in the original isue (#11671) libdir is changed between configuration and installation? What should be fixed for this PR to be accepted? |
|
Ping @alexcrichton, what's the next step on this PR? |
|
@jauhien oh dear I'm sorry sorry this sat for so long, this completely slipped my mind! Having re-read everything I think I understand a little better what's going on here, but there's two changes I think that need making:
Does that sound ok to you? |
|
@jauhien What's your status? |
|
I'll look at it when I'll have time. It should be in the nearest days. |
|
@alexcrichton: your proposition looks ok for me. I will change the PR appropriately. To summarize and be sure that I understand it the right way:
|
not in hardcoded libdir path. If there was no LIBDIR provided during configuration fallback to hardcoded paths. Thanks to Jan Niklas Hasse for solution and to Alex Crichton for improvements. Closes rust-lang#11671
|
Changes pushed. As I have said: it ignores --libdir for windows and determines the libdir in the old way if relative libdir is @alexcrichton: please, review this new version. |
|
@alexcrichton: also, please, test this change on windows, as I have no machine with this OS here. |
| return primary_libdir_name(); | ||
| } else { | ||
| return secondary_libdir_name(); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ack sorry about that! I was skimming too quickly :(.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The reasoning is the same as this comment I believe (I'll reply in a just a bit).
|
Ok @jauhien, I think this is good to go. I'd like to confirm though, have you run a full build yet with |
| else | ||
| HLIB$(1)_H_$(3) = $$(HROOT$(1)_H_$(3))/$$(CFG_LIBDIR_RELATIVE) | ||
| endif | ||
| endif |
There was a problem hiding this comment.
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
|
@alexcrichton: yes, I have run a full build with those changes and tested it works. But I will add simplification changes from @jmesmon and test again, so, please, do not merge at the moment. I will push changes and comment here if everything works. @jmesmon: afaik on windows libdir changing does not work correctly. They store libraries in the same |
|
@alexcrichton: this change from @jmesmon did not work for me. I guess, I can fix it, but it is fragile and adds additional problems for the future maintainance of those build scripts (the easier scripts are the better). So you can merge my changes if you have no objections. The only thing: I have tested them only on GNU/Linux, if you need to test it on any other platform, please, do it before merging. |
|
I'm not asking you to change the use of Even if windows can't support it, I'd rather keep logic around OS specifics in the configure script to a minimum. |
|
@jmesmon currently the libdir logic is special for windows because Rust doesn't actually work out-of-the-box if the dlls for librustc and such aren't next to |
|
@jauhien ok, thanks so much for being patient on this! |
Fixies #11671 This commit changes default relative libdir 'lib' to a relative libdir calculated using LIBDIR provided by --libdir configuration option. In case if no option was provided behavior does not change.
|
@alexcrichton: thanks ) |
|
I just tried to make install a build of Rust that has this change on Windows 8.1 x64 using MSYS2 and the only flags I passed to Do I have to explicitly pass |
|
Mmm, strange, as it should set |
|
Yes, I know what was the problem: just ignoring on windows was a bad idea, as it does not write info to configured variables. I will make a PR with fix in some time. |
These slots are added: (major.minor) rust release (nightly) nightly version (git) git master branch eselect module was added, that allows switching between different rust versions To make all this work, proper support of --libdir option in rust build system was implemented (previous version did not work), see rust-lang/rust#16552 Until upstream accepts it, it will be maintained downstream
Fixies #11671
This commit changes default relative libdir 'lib' to a relative libdir calculated using LIBDIR provided by --libdir configuration option. In case if no option was provided behavior does not change.