-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
brotli: ensure CMAKE_SYSTEM_NAME=Windows on mingw cross builds #46773
brotli: ensure CMAKE_SYSTEM_NAME=Windows on mingw cross builds #46773
Conversation
CMAKE passes the flag -rdynamic otherwise (which is not recognized by mingw). Setting the appropriate flag avoids this problem and allows the build to succeed.
For reference, here's the shell.nix file I used for testing: with import <nixpkgs> {};
{
my-env = pkgs.stdenv.mkDerivation {
name = "my-env";
buildInputs = [
pkgs.pkgsCross.mingwW64.brotli
];
};
} |
Thanks! I'm glad someone is also working with Mingw. I think we can actually set this up for all CMake builds. Reading the documentation, it looks like by default CMAKE_SYSTEM_NAME is set to CMAKE_HOST_SYSTEM_NAME (which awkwardly is a different kind of "host" compared to the one in Nixpkgs): https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_NAME.html The only thing is keeping track of nixpkgs/pkgs/development/tools/build-managers/cmake/setup-hook.sh Lines 32 to 33 in 92a047a
|
@GrahamcOfBorg build pkgs.pkgsCross.mingwW64.brotli |
Success on x86_64-linux (full log) Attempted: pkgs.pkgsCross.mingwW64.brotli Partial log (click to expand)
|
Yeah |
Success on aarch64-linux (full log) Attempted: pkgs.pkgsCross.mingwW64.brotli Partial log (click to expand)
|
Success on x86_64-darwin (full log) Attempted: pkgs.pkgsCross.mingwW64.brotli Partial log (click to expand)
|
Many thanks for the review @matthewbauer @Ericson2314 - I agree it would be ideal if this were to be fixed for all packages at once via the cmake setup hook. I would be happy to try to contribute a patch to that effect. However, I'm not that experienced with setup hooks, so please forgive me for I cannot see immediately how to easily detect within a setup hook that we are running a mingw build (or more generally, that we are running a cross build and what the target then is). I've grep'd all current setup hooks in the repository for inspiration, to no avail. Suggestions? |
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.
Looks okay to merge right now (not sure if it should be master or staging though). Discussing with @Ericson2314 how we can do this everywhere - it gets a little complicated because now we need some kind of a wrapper to hold what the targeted system is, previously we reused the same thing every time. Anyway, it's used in other places already so should be okay.
Maybe I would add this version to 18.09, and do the wrapper on maste/staging. |
I am still wanting to explore ways to avoid the wrapping of cmake. The setup hook should have enough information to get the target from the c compiler without a wrapper. Somehow parsing dumpmachine output? Or some other variable |
I just don't see how computing the same information every time dynamically is superior to doing it once statically. The wrapper would be a lot simpler than cc-wrapper. It can just be the setup hook and a propagated dependency, in fact. |
I agree. Merge this now and generalize after. |
It's possible to create the setup hook once, based on the platform, but I don't see a simple nice way immediately. |
Motivation for this change
CMAKE passes the flag -rdynamic otherwise (which is not recognized by mingw). Setting the appropriate flag avoids this problem and allows the build to succeed.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)I've also tested the resulting executable using wine on fedora (compressing & decompressing a file, checked the difference).
@vcunat @Ericson2314