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: bail upon seeing git dependencies with install scripts #204877

Conversation

winterqt
Copy link
Member

@winterqt winterqt commented Dec 6, 2022

Description of changes
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.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@winterqt
Copy link
Member Author

winterqt commented Dec 6, 2022

This is basically done, I just don't feel like writing a proper description right now, so a draft it is.

@winterqt
Copy link
Member Author

winterqt commented Dec 6, 2022

Maybe I should have ran the tests on the last commit before pushing... oops.

So, as far as I know, there are some dependencies (like node-sqlite3 here) where their install script runs fine. So, we have two options here:

  1. Warn instead of bailing
  2. Bail, but allow the user to say "I want to try to see if this dependency works," via an environment variable or something

Would appreciate any thoughts on this.

@lilyinstarlight
Copy link
Member

So, as far as I know, there are some dependencies (like node-sqlite3 here) where their install script runs fine. So, we have two options here:

  1. Warn instead of bailing
  2. Bail, but allow the user to say "I want to try to see if this dependency works," via an environment variable or something

Would appreciate any thoughts on this.

I'm slightly inclined towards warning if it's non-fatal enough of the time and the warning would be somewhat prominent/not buried in the middle of logs

What is an example of a dependency that it breaks for?

(Also I'll do some actual testing on this PR in a bit, hopefully today or tomorrow)

@winterqt
Copy link
Member Author

winterqt commented Dec 7, 2022

What is an example of a dependency that it breaks for?

https://github.com/mongodb-js/electron-notarize, depended on by https://github.com/mongodb-js/mongosh.

@winterqt winterqt mentioned this pull request Dec 9, 2022
13 tasks
@winterqt
Copy link
Member Author

Superseded by #206476.

@winterqt winterqt closed this Dec 17, 2022
@winterqt winterqt deleted the prefetch-npm-deps-bail-on-git-install-scripts branch December 17, 2022 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants