mesa_noglu: fix build #52772
mesa_noglu: fix build #52772
Conversation
Mesa requires AMDGPU target but previously, we only allowed a pre-defined set of targets.
jtojnar
requested review from
alyssais,
basvandijk,
edolstra,
Ericson2314,
FRidh,
infinisil,
LnL7,
matthewbauer,
Mic92,
nbp,
peti,
ryantm,
ttuegel and
zimbatm
as code owners
GrahamcOfBorg
added
6.topic: erlang
6.topic: GNOME
jtojnar
removed
6.topic: erlang
6.topic: GNOME
| @@ -46,6 +46,13 @@ let | |||
| optionals stdenv.isLinux (if (stdenv.isAarch32 || stdenv.isAarch64) | |||
| then [] | |||
| else ["intel"] ++ lib.optional enableRadv "radeon"); | |||
| llvm = llvmPackages.llvm.override { | |||
There was a problem hiding this comment.
You can do
llvm.override (old: {
enableTargets = old.enableTargets ++ [ "AMDGPU" ];
})There was a problem hiding this comment.
That does not seem to work for me: attribute 'enableTargets' missing
There was a problem hiding this comment.
Oh right, because callPackage is blind to ? default ugh.
There was a problem hiding this comment.
Since mesa is a common package many users will end up with two llvm versions.
For that reason, I would propose to include that target by default.
There was a problem hiding this comment.
in pkgs/development/compilers/llvm/7/llvm.nix I would add enableTargets ? [ stdenv.hostPlatform stdenv.targetPlatform "AMDGPU" ]
There was a problem hiding this comment.
mesa uses llvm 6 at the moment.
There was a problem hiding this comment.
Should I add it to both, or switch mesa to LLVM 7?
There was a problem hiding this comment.
For consistency we can add the target to both but keep llvm6 since as default for mesa since we also use it as default in other places.
There was a problem hiding this comment.
For the next release we could look in to making llvm7 the new default since we depend on llvm7 with rustc anyway.
LLVM’s `LLVM_TARGETS_TO_BUILD` build flag defauls to `all`, which contains `AMDGPU` among others. [1] Changes in llvm [2] switched to explicitly listing host and target platforms, excluding the AMDGPU target, which is required for Mesa to build. [1]: https://github.com/llvm-mirror/llvm/blob/db50b6fe399b8ad8caef80fd8ee77607fb051fa5/CMakeLists.txt#L286-L302 [2]: NixOS#52031
jtojnar
force-pushed
the
mesa-noglu-fix
branch
from
73f5c1c
to
ff22d90
Compare
| @@ -2,7 +2,9 @@ | |||
|
|
|||
| rec { | |||
| llvmBackend = platform: | |||
| if platform.parsed.cpu.family == "x86" then | |||
| if builtins.typeOf platform == "string" then | |||
There was a problem hiding this comment.
Fixed. But I still do not like this.
There was a problem hiding this comment.
We could stringify our default targets before passing them to the list, then it won't be part of the public API.
There was a problem hiding this comment.
I think the ability to pass platform attribute sets to enabledTargets is useful and convenient. We might want to move the llvm target name to lib/platforms.nix; it already contains gcc keys. Then we might simplify the function to
llvmBackend = platform:
if builtins.typeOf platform == "string" then
platform
else if stdenv.lib.hasAttrByPath ["llvm" "targetName" ] then
platform.llvm.targetName
else
throw "Unsupported system";There was a problem hiding this comment.
Oh that's a good point. CC @matthewbauer.
|
Let’s just merge it to unbreak staging and we can discuss the proper fix later. |
Motivation for this change
LLVM’s
LLVM_TARGETS_TO_BUILDbuild flag defauls toall, which containsAMDGPUamong others. Changes in llvm switched to explicitly listing host and target platforms, excluding the AMDGPU target, which is required for Mesa to build.I am not sure if passing the extra targets as a string is a good idea but other than keeping a list of all supported targets, I do not really see a solution.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)