-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
zig: 0.9.1 -> 0.10.1 #210324
zig: 0.9.1 -> 0.10.1 #210324
Conversation
It does more than merely add Zig. Why not concentrate this on the llvm? After that I can rebase and test on my already open PR. |
See my note at the top of the PR:
|
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 great. I like what you did with coreutils env.
Sorry, I can't ignore the whole context. If you are saying that I should pretend your PR contains the zig update and nothing else (therefore, you are not willing to take any responsibility about the other commits in this PR), then I should equally pretend this PR is superfluous and close it. What does your "look only to the latest commit please" pull request bring to the table that can't be grafted on the already opened one? |
This is not what I meant. I meant that only the last commit in this PR is really up for review. I also meant that this PR should merge only after #209536 merges. I should have worded it more clearly. I wanted to express this in the GitHub PR tool, but it wouldn't let me pick #209536's branch as the base branch.
Huh? I don't understand. Why should we pretend that my PR is superfluous? The purpose of this PR is to share the Zig update commit (8054072) which I called out twice now. You are free to ignore my PR, but I don't know why you're being what I perceive as hostile (e.g. suggesting my PR be closed).
The already-opened PR (#199947) was owned by you, so I could not modify it (I think). And even if I could, I think it'd be unprofessional for me to change your PR. I tested my PR. Even better, CI can test my PR. If I exclude the LLVM commits (which is what I think you're suggesting), CI could not test my PR. |
@AndersonTorres it is common for a PR to be opened in anticipation of another one being merged. This allows to move forward faster, since it can be reviewed ahead of time, tested ahead of time, and then rebased once the prerequisite PR is merged. You are not being asked to pretend anything; only to wait until the other PR is merged before dealing with this PR. Or, if you feel generous, you can offer review on the last commit in this PR, potentially saving a future review round trip for yourself and for @strager. |
@strager I support you. Have you considered setting the base branch of your PR to #209536 ? That has other weirdnesses too, like someone merging this PR before #209536 is merged would mess that PR up. But it would accurately show the diff. Besides, if you believe this PR shouldn’t be merged now (for some reason) then consider marking it as a Draft until then. Just my opinion, but imo since this PR is getting so much pushback I would change the base branch and mark it as a Draft Edit: I see you did try changing the branch. In that case, forget Anderson |
(or look into a tool like ReviewStack / Sapling) that is more capable of handling these kinds of reviews |
using commit of another PR also has the advantages to test two PR at once. We see that llvm if working fine (ci of my PR) + another project that use it the test suite of zig confirm llvm is working. Here I reviewed this PR and @strager will be able to work on the PR while waiting for llvm to be merged so multithreading :) I tested to build and it seem to fail on x86_64-darwin I got this error from the log:
I can't debug right now (because I already debug another Darwin failure) and it seems more zig specific. I have Darwin sandbox activated. I don't know if the call stack is optimized/normal or a bug |
OK.
In a certain sense, both PRs cannot be merged as is, because they modify the same files, have the same ending purpose, and any one of them can be accepted to the prejudice of the other. The only relevant difference between mine and yours is the cherry-picking of LLVM-related snippets that, according to you, are not the most relevant part of this PR. At least in a large sense, one of them must go. Or, if you think this is too harsh, your PR supercedes or at least is equivalent to mine.
You did not explain what was your purpose when you opened this PR. Your two messages say absolutely nothing besides "hey I opened a PR".
You could cherry-pick the same way you did with the LLVM-related ones. At least I would be able compare the effective differences between them and hopefully merge a conjoined work. |
@@ -59,16 +38,20 @@ stdenv.mkDerivation rec { | |||
export HOME=$TMPDIR; | |||
''; | |||
|
|||
patchPhase = '' |
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.
patchPhase = '' | |
postPatch = '' |
@Et7f3 I tried on my AArch64 macOS machine. I get a different failure:
I'll debug what I see, and maybe that will fix the x86_64 build too. |
0.10.1 will be tagged soon, within 1-2 weeks. This could potentially be fixed by one of the many bug fixes that made it into that tag. You might consider testing with the 0.10.x branch. |
I see the same problem with the 0.10.x branch, but the build works on my MacBook Pro on master (Zig commit 7cb2f9222da38d687e8708dd5d94d3175cc77995). I'm bisecting now. |
Speaking for upstream, unfortunately, there are a lot of bug fixes in master branch that cannot be cherry-picked to 0.10.x because the branches have already diverged quite a bit. So unless the fix is a small patch that can be applied cleanly, we may be stuck with the bug in 0.10.1. I expect the quality of 0.11.0 to be much higher (this is related to deleting the old compiler which happened after releasing 0.10.0). |
If this minor release series will be "irredeemably buggy", should we bring the unstable/master release instead? TBH I was planning this a long time ago, but there was no serious use for a master branch of zig. |
@andrewrk, is there a policy of Long Term Support on Zig? Or an update in a minor version is sufficient to consider it deprecated? |
Zig follows Semantic Versioning. |
zig 0.10.1 released today https://github.com/ziglang/zig/releases/tag/0.10.1 Aside: I tried using this to package some Zig river utils (stacktile and levee) and they error out in the same way ( |
@AndersonTorres do you have the time to review this again or might someone else be able to help out? We at roc-lang are also blocked on this PR and I would really appreciate your input :) |
There are many blockers to solve here. The code does not even compile outside x64-linux. |
I see, in any case, thanks for your quick reply! @strager could it be that using zig 0.10.1 here would fix the issues on darwin? |
Nope. Zig 0.10.1 on Darwin still has issues. |
What blockers are you referring to? |
I updated this PR to use Zig 0.10.1 instead of Zig 0.10.0. |
Please rebase :-) |
Several Zig-using packages are broken with a newer version of Zig, and other packages are blocked on a Zig upgrade. Prepare for two Zig versions side-by-side by renaming default.nix to 0.9.1.nix.
On Linux, upgrade Zig to version 0.10.1. On macOS/Darwin, Zig version 0.10.1 is broken, so keep 0.9.1. Several Zig-using packages are broken with Zig version 0.10.1, so pin those packages to Zig version 0.9.1.
LLVM 15 landed in #194634. I rebased my PR on top of it. |
@AndersonTorres Is this PR good to go? |
LGTM |
ncdu 2.x is written in zig [1]. Zig is not stable. It was recently updated from 0.9.x to 0.10.x [2]. Some logic tried to keep 0.9.x on macOS [3], but it did not work [4]. ``` error: Package ‘zig-0.10.1’ in /nix/store/bqq0f9qwk3nn3icrzl5sfpmfbqb6yz95-nixpkgs/pkgs/development/compilers/zig/0.10.nix:59 is marked as broken, refusing to evaluate. ``` [1] https://dev.yorhel.nl/doc/ncdu2 [2] NixOS/nixpkgs#210324 [3] NixOS/nixpkgs@db76c9e [4] NixOS/nixpkgs#214545
Description of changes
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes