Conversation
github-actions
bot
added
10.rebuild-darwin: 1
|
|
Would this also fix #381980 ? |
liberodark
force-pushed
the
wrangler
branch
2 times, most recently
from
7473c21 to
1fcff8a
Compare
|
|
|
Here is the error I ran into
this appears to be a pnpm error. |
I'm getting that as well, also this error on my flake as well: https://github.com/ryand56/wrangler/actions/runs/13279400288/job/37074675503#step:5:209 |
|
|
With the last release now we have :
|
|
Im not sure is ok to use : |
I think it should be fine to use but it should be considered temporary - if you can figure out how to fix the missing symlinks that would be the preferred solution |
liberodark
force-pushed
the
wrangler
branch
2 times, most recently
from
3446912 to
3224c1b
Compare
|
| nativeInstallCheckInputs = [ | ||
| versionCheckHook | ||
| ]; | ||
| versionCheckProgramArg = "-v"; | ||
| doInstallCheck = true; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for your help, I do appreciate being explicit and yes it is intentional.
There was a problem hiding this comment.
Some programs like to print their full path somewhere in --help, which would make the check obsolete. So it's always a good idea to specify the argument.
There was a problem hiding this comment.
It seems like --version should take precedence over --help to minimize the likelihood of this, but I agree it is ultimately best to just be explicit.
cameronraysmith
added
the
12.approvals: 2
ryand56
left a comment
ryand56
left a comment
There was a problem hiding this comment.
nixpkgs-review result
Generated using nixpkgs-review.
Command: nixpkgs-review pr 377334
Commit: 2edbc076f0727ed3c10554429a02ab2870a6f5f5
x86_64-linux
✅ 1 package built:
- wrangler
LGTM, basic functionality works
ryand56
added
12.approvals: 3+
|
from the resolved thread
|
|
Several people are involved in this PR, so i want to make sure im not doing anything against anyones wishes. Im planning to merge this in 24h unless someone disagrees. |
|
@liberodark could you also add me ( |
|
cameronraysmith
left a comment
cameronraysmith
left a comment
There was a problem hiding this comment.
Approved automatically following the successful run of nixpkgs-review.
I think it's best if you do a PR to add yourself. |
Is good to me |
|
I'm making an assumption here but based on the changes you cherry picked ezrizhu@042acb0 but left no credit? Can we at least co-author @ezrizhu. |
Hi, Best Regards |
Yep I hadn't even seen the date. PS : In my mind have keep only that : |
There was a problem hiding this comment.
i'll bite the bait
that screenshotted commit with the short writeup - c2ef848 - was made after your original commit with my diffs (8465fe6), which appears to be a line-by-line copy of my below two commits from yesterday
Lines in question that is quite obvious:
8465fe6#diff-62f7294149d81149b2724158561ac4a54a57ab507239fe806de590326191c1b5R39-R74 (comment on line 74)
8465fe6#diff-62f7294149d81149b2724158561ac4a54a57ab507239fe806de590326191c1b5R62 (the mv line on line 62)
8465fe6#diff-62f7294149d81149b2724158561ac4a54a57ab507239fe806de590326191c1b5R34 (the sed line on line 34)
sure the sed line is arguable but the Update: comment? c'mon..
unrelated to above, could you update the commit msg to have below so the reasons for the change is in the git log tree.
* Broken symlinks: nixpkgs build now checks for broken symlinks, this PR
adds all the relevant directories back to the out dir so all symlinks are
intact.
* Missing npm: wrangler specifies an older version of pnpm than the one in
pnpm_9 as used in the build process, this PR patches the package.json to remove
the pnpm version so pnpm_9 doesn't attempt to change it's version resulting in a
pnpm binary not found error.
and if you want to actually credit me, append below that bit. (i don't care that much about the commit credits, but at least acknowledge you blatantly copied it. not inspired...
Co-authored-by: Ezri Zhu <me@ezrizhu.com>
im unsubbing from this thread, this has done nothing but to detract from my day. I am very new to nixpkgs but if the culture is always like this then I don't see a point in me trying to learn to contribute to this repo any further.
EDIT:
I have no qualms with you going ahead with your PR, but I do ask that you're at least receptive of my feedback and add the other part of my commit that you at least saw, which is the explanation in the commit message. I think the explanation being in the tree adheres to the commit conventions better and saves future maintainers the hassle of digging up this thread that is very long.
PS: I go by she/her. Please don't refer to me like that again.
|
Hi, @ezrizhu I think you misunderstood, honestly. Best Regards |
Also resolved below two build issues * Broken symlinks: nixpkgs build now checks for broken symlinks, this PR adds all the relevant directories back to the out dir so all symlinks are intact. * Missing npm: wrangler specifies an older version of pnpm than the one in pnpm_9 as used in the build process, this PR patches the package.json to remove the pnpm version so pnpm_9 doesn't attempt to change it's version resulting in a pnpm binary not found error. (with help from @cameronraysmith in NixOS#377334)
|
@liberodark @ezrizhu the miscommunication here is unfortunate and I hope it will not discourage either of you or others who review the discussion from contributing productively and in good faith in the future. I apologize if I misunderstood some important aspects of the history in association with #409499 in my enthusiasm to help get these updates reviewed. In my defense, #409499 did not appear referenced directly in the thread here, even via backlink, until it was closed. I am glad a solution has been merged in #409572 . opinion on low threshold for acknowledging shared contribution
@liberodark I think it probably makes sense to have a very low threshold to note the source and add co-authors to commits, especially if someone has ever put up a parallel PR even if you did not know about it, or, more relevant to this case, requests to be acknowledged with a very clear history of productive participation. You can usually get a user's github e-mail with Co-authored-by: ezrizhu <44515009+ezrizhu@users.noreply.github.com>
Co-authored-by: liberodark <4238928+liberodark@users.noreply.github.com>to the commit to credit them without any risk to their privacy as long as you're careful to avoid signing someone's name on something they might actively disagree with. Thank you for your contributions! from competing parallel PRs to a stack-first in review mentality
Personally, I think a deeper problem here is with github ;)
If @ezrizhu could have more easily suggested changes to lines that had not previously been edited by simply providing a patch file from her branch, it could often avoid similar scenarios where it seems there is a need to put up a parallel PR when one has already existed for a long period of time. I suppose you can submit a PR to the OPs fork targeting the PR source branch, which would create a clear historical record and update the older one if merged. Hmm, that actually kind of seems like a reasonable solution: https://github.com/orgs/community/discussions/4452#discussioncomment-13227929 . Wow, this has actually happened before on your fork @liberodark
Kudos to you |
Also resolved below two build issues * Broken symlinks: nixpkgs build now checks for broken symlinks, this PR adds all the relevant directories back to the out dir so all symlinks are intact. * Missing npm: wrangler specifies an older version of pnpm than the one in pnpm_9 as used in the build process, this PR patches the package.json to remove the pnpm version so pnpm_9 doesn't attempt to change it's version resulting in a pnpm binary not found error. (with help from @cameronraysmith in NixOS#377334) (cherry picked from commit 515e5fe)
13 participants
Fix :
Information :
adds all the relevant directories back to the out dir so all symlinks
are intact.
pnpm_9 as used in the build process, this PR patches the package.json
to use the pnpm version that is currently present in the build
environment, this way when we update pnpm_9 again wrangler will still
build.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.