-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
zig: use setupHook attribute on zig derivation #473413
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
Conversation
jcollie
left a comment
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.
Ghostty changes LGTM
bb5b2d0 to
18ef953
Compare
18ef953 to
2be232b
Compare
|
|
looks like roc is failing to build on master as well |
|
|
Bash complains about |
2be232b to
2910927
Compare
|
I got different results 🤔 |
|
@RossComputerGuy any chance you could take another look at this? I didn't seem to run into the same breakages you did. |
kirillrdy
left a comment
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.
nixpkgs-review result
Generated using nixpkgs-review.
Command: nixpkgs-review pr 473413
Commit: 440c93a747de74f188145f6abb2f2771d0f5f458
x86_64-linux
✅ 57 packages built:
- arocc (aroccPackages.latest)
- aroccPackages.latest-unwrapped
- aroccStdenv
- blackshades
- bork
- cargo-lambda
- cargo-zigbuild
- creek
- dt
- findup
- flow-control
- ghostty
- ghostty.man
- ghostty.shell_integration
- ghostty.terminfo
- ghostty.vim
- glsl_analyzer
- hevi
- linuxwave
- lsr
- ly
- mepo
- minizign
- ncdu
- nixpkgs-manual
- odiff
- outfieldr
- owi
- pkgsArocc.stdenv
- pkgsZig.stdenv
- poop
- river-bedload
- river-classic
- river-classic.man
- river-ultitile
- rivercarro
- roc
- superhtml
- tuatara
- waylock
- wayprompt
- zf
- zig (zig_0_15)
- zig-zlint
- zig.doc (zig_0_15.doc)
- zigStdenv
- zig_0_13
- zig_0_13.doc
- zig_0_14
- zig_0_14.doc
- zigfetch
- zigimports
- zls (zls_0_15)
- zls_0_14
- zon2nix
- zsnow
- ztags
|
|
@ofborg build wayprompt |
|
Huh, I still get the bash problem where it doesn't like |
|
I haven't personally ran into the build failures nor the bash issue. Ofborg doesn't seem to fail builds either (tested with just wayprompt, https://logs.ofborg.org/?attempt_id=e1f15a20-9be5-4213-b236-3e5a06287231&key=nixos%2Fnixpkgs.473413). Does it seem fair to chalk those issues up to something with your particular setup? |
|
Idk, I don't use bash so bash is stock. This is the only PR I've seen this issue with. |
|
Spoke offline with @RossComputerGuy, we'll go ahead with the merge here as it's likely the breakage in his review was due to using a kernel built with 64K page size, which is known to not work well on older zig releases. |
By moving the zig setup hook to the zig derivation itself, we allow for zig to splice correctly with `callPackage`, meaning that the correct zig can be pulled in during builds when zig is in `nativeBuildInputs` (for example). This change retains the `zig.hook` attribute for backward compatibility by just pointing to the zig derivation. This also removes `zig_default_flags`, since now the setup hook is not a derivation that can be overridden. Overriding the build flags can now be done by setting `dontSetZigDefaultFlags = true`.
As of NixOS/nixpkgs#473413[1], `zig.hook` is no longer necessary, and optimization flags can now be included directly in `zigBuildFlags`. [1] NixOS/nixpkgs#473413
As of NixOS/nixpkgs#473413[1], `zig.hook` no longer supports `zig_default_flags`, and now they can and must be provided in `zigBuildFlags` instead. [1] NixOS/nixpkgs#473413
As of NixOS/nixpkgs#473413[1], `zig.hook` no longer supports `zig_default_flags`, and now they can and must be provided in `zigBuildFlags` instead. Updating also requires removing gnome-xorg since it has been removed from nixpkgs. [1] NixOS/nixpkgs#473413
As of NixOS/nixpkgs#473413[1], `zig.hook` no longer supports `zig_default_flags`, and now they can and must be provided in `zigBuildFlags` instead. Updating also requires removing gnome-xorg since it has been removed from nixpkgs. [1] NixOS/nixpkgs#473413
As of NixOS/nixpkgs#473413[1], `zig.hook` no longer supports `zig_default_flags`, and now they can and must be provided in `zigBuildFlags` instead. Updating also requires removing gnome-xorg since it has been removed from nixpkgs. [1] NixOS/nixpkgs#473413
|
👋 I am not so much a nix expert here, but I think this is backwards incompatible change for existing uses of zig in devShells. I think this surfaces this issue more bluntly: #270415 Since now there's not a separate I worked around this by adding |
|
It is a backwards incompatible change, that's why this PR isn't being backported to a stable release. I haven't looked into that issue that much but I've definitely seen it before. I am not sure why it is happening that way. But anyone is free to look into this and submit PR's. |
|
#479423 addresses this by setting that environment variable in configurePhase |
As of NixOS/nixpkgs#473413, `zig.hook` no longer supports `zig_default_flags`, and now they can and must be provided in `zigBuildFlags` instead. Updating also requires removing gnome-xorg since it has been removed from nixpkgs. `nix flake check` succeeds on my system (x86_64-linux), with a couple deprecation warnings that I believe aren't important.
By moving the zig setup hook to the zig derivation itself, we allow for zig to splice correctly with
callPackage, meaning that the correct zig can be pulled in during builds when zig is innativeBuildInputs(for example).This change retains the
zig.hookattribute for backward compatibility by just pointing to the zig derivation. This also removeszig_default_flags, since now the setup hook is not a derivation that can be overridden. Overriding the build flags can now be done by settingdontSetZigDefaultFlags = true.Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.