-
-
Notifications
You must be signed in to change notification settings - Fork 16.7k
llvmPackages.libunwind: fix Windows paths #418054
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
base: master
Are you sure you want to change the base?
llvmPackages.libunwind: fix Windows paths #418054
Conversation
Symlink the shared libraries for Windows properly to unstick the cross-building of the packages
+ lib.optionalString (doFakeLibgcc && stdenv.hostPlatform.isWindows) '' | ||
ln -s $out/bin/libunwind.dll $out/bin/libgcc_s.so | ||
ln -s $out/bin/libunwind.dll $out/bin/libgcc_s.so.1 |
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.
It probably is just easier to use stdenv.hostPlatform.extensions.library
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.
That doesn't account for bin
vs 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.
If the problem was that you need to find the linker target under the name libgcc_s
then we can symlink .dll.a
from within the lib
directory. If the problem is finding the runtime shared libraries, then this needs to be symlinking the dll
files, which Windows keeps in bin
.
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 wonder if stdenv.hostPlatform.libDir
should be changed to handle this, probably should?
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.
Currently it seems to be null
for the ucrtAarch64 platform, so that's not good! But Windows is a strange case. Because the shared libraries typically go under bin
because they are considered runtime and Windows only searches alongside the executable binary and in c:\Windows
for them. While both static libraries (extension .a
usually for gcc/llvm) and the shared library linker stubs (extension .dll.a
for gcc/llvm) both typically live under lib
by convention as they are required at link time.
If you remember which is necessary for making Rust happy, I can make this PR target the appropriate files. But so far I have not heard anyone in this repo reach consensus on a way to make this work generically for Windows cross build targets and *nix hosts.
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.
That is why, in order to know how to properly handle this, we need to know from @RossComputerGuy whether the symlinks he is creating were to address link-time issues or runtime issues.
It was a build failure of Rust libraries, so I think it would be link time.
It is also why a simple, straightforward substitution of variables into the paths for this sort of workaround has not yet been arrived at which can handle both Windows and Unix targets. Windows has an entire additional case to consider that is a moot point in Linux, as linking targets on Linux can be done by pointing directly to the shared library but for whatever reason that can't be (or at least isn't being) done on Windows builds.
So that sounds to me like the right thing to do might be to have stdenv.hostPlatform.extensions.sharedLibraryLink
and stdenv.hostPlatform.extensions.sharedLibraryRuntime
, which are the same on Linux but different on Windows, and then deprecate stdenv.hostPlatform.extensions.sharedLibrary
?
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.
So are you foreseeing that those would be functions that would return lib/$library.dll.a
and bin/$library.dll
on Windows but would both return $prefix/lib/$library.so
on other targets? So they'd be invoked as stdenv.hostPlatform.extensions.sharedLibraryLink "libfoo"
or similar? If so it would result in duplicated code on non-Windows targets. Not terrible if the command is ln
but sometimes it's a mv
or rm
command. rm
can be told to ignore failures with -f
but I don't know if mv
can be similarly invoked.
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.
So is there a hook we could have to handle this in a standard-ish way, so each package doesn't have to right that code itself?
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'm not completely familiar with them - what I understand of them is that they're shell scripts that run during the phases automatically when they're in the inputs list. Those could cover most of the standard conditions, for instance if they're using an older autotools and it puts the libraries into lib/libfoo.dll
instead of bin/libfoo.dll
. But for a case like the one above, where libunwind
is being symlinked to libgcc_s
, how would that be indicated to the hook?
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.
In this case I think we'd just do an ln
using sharedLibraryLink
or whatever. More generally, hopefully most of the time we could use a hook so we didn't end up with duplicated code on non-Windows targets as you mentioned above.
Symlink the shared libraries for Windows properly to unstick the cross-building of the packages
This bug was introduced in #330037.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.