Skip to content
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

prefetch-npm-deps: read url bodies within the retry loop #260011

Conversation

lilyinstarlight
Copy link
Member

@lilyinstarlight lilyinstarlight commented Oct 9, 2023

Description of changes

Fixes issues with prefetch-npm-deps not properly retrying on network errors by reading the full body within the retry loop (rather than processing the request in the retry loop but then reading the body after)

Closes #257805
Closes #256873
Closes #258428

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@Gerg-L
Copy link
Contributor

Gerg-L commented Oct 9, 2023

works for me

$ nix build github:nixos/nixpkgs#bitwarden --option substituters ''
error: builder for '/nix/store/a49pbxzg99z3j8yi8n27ibb1r0w73r90-bitwarden-2023.9.0-npm-deps.drv' failed with exit code 1;
       last 10 log lines:
       > node_modules/dir-compare/node_modules/minimatch
       > node_modules/through2-filter
       > node_modules/static-module/node_modules/magic-string
       > node_modules/@babel/plugin-transform-unicode-sets-regex
       > node_modules/@babel/plugin-syntax-export-namespace-from
       > node_modules/stat-mode
       > Error: unknown error
       >
       > Caused by:
       >     [55] Failed sending data to the peer
       For full logs, run 'nix log /nix/store/a49pbxzg99z3j8yi8n27ibb1r0w73r90-bitwarden-2023.9.0-npm-deps.drv'.
error (ignored): error: cannot unlink '/tmp/nix-build-openssh-9.5p1.drv-0': Directory not empty
error: 1 dependencies of derivation '/nix/store/531n38krd1jxbycp954sg5cm1fp2p6w4-bitwarden-2023.9.0.drv' failed to build
$

vs

$ nix build github:lilyinstarlight/nixpkgs/fix/prefetch-npm-deps-network-error-recovery#bitwarden --option substituters ''
$

Copy link
Member

@amarshall amarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran nix-build -A bitwarden.npmDeps --check 20 times on 48 cores with no failures! Thanks!

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 10, 2023
Copy link
Contributor

@Gerg-L Gerg-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@delroth delroth added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Oct 10, 2023
@lilyinstarlight lilyinstarlight force-pushed the fix/prefetch-npm-deps-network-error-recovery branch from 9c8fdbf to 554e241 Compare October 10, 2023 23:41
@lilyinstarlight
Copy link
Member Author

That last push was to change the method name a bit to clarify its change in behavior. It should just be a cosmetic/semantic change rather than a functional one

@delroth delroth removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Oct 11, 2023
@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Oct 11, 2023
@wegank wegank mentioned this pull request Oct 11, 2023
12 tasks
@delroth delroth added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Oct 11, 2023
@wegank wegank merged commit 093f098 into NixOS:master Oct 12, 2023
@github-actions
Copy link
Contributor

Backport failed for release-23.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-23.05
git worktree add -d .worktree/backport-260011-to-release-23.05 origin/release-23.05
cd .worktree/backport-260011-to-release-23.05
git checkout -b backport-260011-to-release-23.05
ancref=$(git merge-base a2b87a4f66f309d2f4b789fd0457f5fc5db0a9a6 554e2412e0b4a89f527786e67d568d9cf3842d80)
git cherry-pick -x $ancref..554e2412e0b4a89f527786e67d568d9cf3842d80

@lilyinstarlight
Copy link
Member Author

Manual backport is in #260742 (backport failure was due to #255168 not having been merged, but #260742 supersedes that one now)

@lilyinstarlight lilyinstarlight deleted the fix/prefetch-npm-deps-network-error-recovery branch October 12, 2023 18:33
@amarshall amarshall mentioned this pull request Oct 23, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nodejs 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people
Projects
Status: Done
5 participants