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

jellyfin-web: convert to buildNpmPackage #200475

Closed

Conversation

winterqt
Copy link
Member

@winterqt winterqt commented Nov 10, 2022

Description of changes

Over 14,000 deletions ✨

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

Copy link
Contributor

@tjni tjni left a comment

Choose a reason for hiding this comment

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

This looks really nice. Great work!

pkgs/build-support/node/fetch-npm-deps/src/main.rs Outdated Show resolved Hide resolved
pkgs/build-support/node/fetch-npm-deps/src/main.rs Outdated Show resolved Hide resolved
pkgs/build-support/node/fetch-npm-deps/src/main.rs Outdated Show resolved Hide resolved
pkgs/build-support/node/fetch-npm-deps/src/main.rs Outdated Show resolved Hide resolved
pkgs/build-support/node/fetch-npm-deps/src/main.rs Outdated Show resolved Hide resolved
pkgs/build-support/node/fetch-npm-deps/src/main.rs Outdated Show resolved Hide resolved
@minijackson
Copy link
Member

Hey! Thank you for the PR! First let me thank you for your work on buildNpmPackage, it looks amazing and quite handy.

But at the risk of being a joy killer, is this what we want for jellyfin-web? I think buildNpmPackage is amazing for rapid packaging, and in the case where node2nix isn't feasible. The reason I think that we still want to use node2nix is for transparency / auditability: doing nix path-info -r --derivation on jellyfin-web with node2nix does tell you which NPM packages are used, and you can automatically audit if some of them have CVEs (with vulnix for example). Having a FOD makes us lose that property, unfortunately.

I do love the amount of deletion, but since a lot of lines are generated code, I wouldn't consider them a maintenance burden or technical debt. The web.nix and web-update.sh files seem quite simple to me.

What are you thoughts on this?

On a lighter note, just the fact that you managed to package the jellyfin-web that quickly is a feat in my opinion, congrats!

@winterqt
Copy link
Member Author

winterqt commented Nov 10, 2022

@tjni Do you mind moving your comments over to #200470 (which is where these changes are from), so I can reply in context? Thanks!

@minijackson That's definitely a valid concern. However, at the same time, I don't see what makes jellyfin-web different from any other npm package that will be converted in the future. Maybe we should wait on others to weigh in?

@minijackson
Copy link
Member

@winterqt yes, sorry for the confusion, I wasn't talking about jellyfin-web specifically. My point of view would be more like "for nixpkgs, try node2nix. If it doesn't work for you switch to buildNpmPackage"

@winterqt
Copy link
Member Author

for nixpkgs, try node2nix. If it doesn't work for you switch to buildNpmPackage

I disagree for a few reasons, mainly that the current package set (node-packages.nix):

  • takes hours to generate
  • updates affect other packages
  • is very large

@minijackson
Copy link
Member

Isn't the package set for jellyfin self-contained in its own node-deps.nix? Or are there some dependencies pulled from the global node-packages.nix as well? I'm not very familiar with the Node ecosystem of nixpkgs.

@winterqt
Copy link
Member Author

You're correct, but you said that you weren't talking about jellyfin-web specifically.

@minijackson
Copy link
Member

I was talking about packages inside nixpkgs. I think all of them can have their own node-deps.nix, and we could forgo the global node-packages.nix? (This is complete speculation)

@winterqt
Copy link
Member Author

Yes, but we really don't want to have so many large generated files if we can help it.

@RaitoBezarius
Copy link
Member

I was talking about packages inside nixpkgs. I think all of them can have their own node-deps.nix, and we could forgo the global node-packages.nix? (This is complete speculation)

Some people are annoyed and find problematic that nixpkgs is holding so much data due to lockfiles appearing from everywhere in the repository, I am sympathetic to the issue, even if I am not really affected as I have plenty of disk space and fiber-speed bandwidth more or less everywhere.

Maybe, there should be a discussion on (a) what is the objective of lockfile reduction in nixpkgs? (b) how to balance this out with our objective of observability/SBOMs/supply chain security/transparency/auditability/etc. ?

@pbsds
Copy link
Member

pbsds commented Nov 26, 2022

Maybe, there should be a discussion on (a) what is the objective of lockfile reduction in nixpkgs? (b) how to balance this out with our objective of observability/SBOMs/supply chain security/transparency/auditability/etc. ?

Odd idea for (b): how about dumping a list of dependencies in a separate output?

@RaitoBezarius
Copy link
Member

Maybe, there should be a discussion on (a) what is the objective of lockfile reduction in nixpkgs? (b) how to balance this out with our objective of observability/SBOMs/supply chain security/transparency/auditability/etc. ?

Odd idea for (b): how about dumping a list of dependencies in a separate output?

It would be an idea, involving @nikstur as he has been working on https://github.com/nikstur/bombon for which this can be of interest.

Also, there was the idea of having a passthru.ssc.cycloneDX thing going in the NixOS SLSA matrix channel.

@pbsds pbsds mentioned this pull request Nov 26, 2022
13 tasks
@winterqt winterqt force-pushed the jellyfin-web-build-npm-package branch from ae0b8a1 to aba3af9 Compare December 17, 2022 04:11
@winterqt winterqt changed the title jellyfin-web: migrate to buildNpmPackage jellyfin-web: convert to buildNpmPackage Dec 17, 2022
@winterqt winterqt marked this pull request as ready for review December 17, 2022 04:12
@winterqt winterqt force-pushed the jellyfin-web-build-npm-package branch from aba3af9 to f4aabbc Compare December 17, 2022 04:15
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2023
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@erictapen
Copy link
Member

I think this is very much outdated by now? jellyfin-web seems to use buildNpmPackage.

@erictapen erictapen closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants