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

bgpalerter: init at 1.28.1 #115474

Closed
wants to merge 2 commits into from
Closed

Conversation

zhaofengli
Copy link
Member

Motivation for this change

BGPAlerter is a self-configuring BGP monitoring tool.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

buildPhase = ''
runHook preBuild

ln -s ${buildEnv}/lib/node_modules node_modules
Copy link
Member

Choose a reason for hiding this comment

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

6000 lines is a too much code for a single package. Can you look pkgs/development/node-packages/node-packages.json on how to create build dependencies that gets added shared node packages set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, that's done. I've added the new packages in a separate commit as the regenerated file affects other packages as well.

@zhaofengli zhaofengli force-pushed the bgpalerter branch 3 times, most recently from 50f42a5 to c26860d Compare March 9, 2021 23:52
@zhaofengli
Copy link
Member Author

Added the dependencies into the shared node package set as suggested, and cleaned up the expression. The new generated node package set will affect other packages so I've added it in a separate commit.

@siraben
Copy link
Member

siraben commented Jun 29, 2021

@zhaofengli please resolve the merge conflicts.

@Mic92
Copy link
Member

Mic92 commented Jun 29, 2021

I rebased the bgpalerter here: https://github.com/Mic92/nixpkgs/tree/bgpalerter-rebased
I only solved some merge conflicts and regenerated node-packages

@zhaofengli
Copy link
Member Author

Hi, sorry, been using bgpalerter in an overlay with yarn2nix and haven't kept this PR up to date. Let me test and push the rebased version from @Mic92 in a bit.

@zhaofengli
Copy link
Member Author

Well, 1.27.1 with the latest nodePackages fails with the use of a deprecated function:

WARNING: the generated configuration is a snapshot of what is currently announced. Some of the prefixes d
on't have ROA objects associated or are RPKI invalid. Please, verify the config file by hand!
Done!
Error: The file prefixes.yml is not valid yml: Function yaml.safeLoad is removed in js-yaml 4. Use yaml.l
oad instead, which is now safe by default.
    at _loop (/nix/store/iq2y43lb2k3wyiajp8sgx0q266k61yqx-bgpalerter/share/bgpalerter/src/inputs/inputYml
.js:131:24)
    at /nix/store/iq2y43lb2k3wyiajp8sgx0q266k61yqx-bgpalerter/share/bgpalerter/src/inputs/inputYml.js:191
:24
    at new Promise (<anonymous>)
    at InputYml._loadPrefixes (/nix/store/iq2y43lb2k3wyiajp8sgx0q266k61yqx-bgpalerter/share/bgpalerter/sr
c/inputs/inputYml.js:109:14)
    at /nix/store/iq2y43lb2k3wyiajp8sgx0q266k61yqx-bgpalerter/share/bgpalerter/src/inputs/inputYml.js:75:
24
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)

This is where I want to bring this HN comment up. To quote the author:

For example, when I was working on Nix, the canonical mechanism for updating a single Node package in the Nix repository was to run a script that updated all (yes, all) Node packages in the repo. When I pointed out all the ways that this was an impractical approach, the response was that it's actually a good thing because that way the Node packages in the repo don't get get stale (!) This was one of a few examples I encountered where the devs didn't seem to be aware of how unusable Nix was in real world cases.

I have to agree with their point here. We want to deliver upstream's original intent, and when the upstream clearly specifies which exact versions of dependencies they want to use in their lock file, we should respect that and use those exact versions. Yes, it's deprecated. And yes, we should bump up to 1.28.1 and things will be alright. But this whole thing is impossible to maintain without silently breaking things everywhere (here the program starts but does not work). BGPAlerter is part of my monitoring system, and I expect it to work and keep working.

Not a dig at the JavaScript folks, but npm is not a very disciplined ecosystem of packages. Many packages there are goofy and don't care about semver. And here we are, ignoring semver completely (this is even worse than pythonPackages where we at least do things semi-manually).

The developer experience is abysmal as well, to say the least. This PR and #126664 are just two examples of endless merge conflicts resulting from frequent completely unchecked change-the-world updates to a single file. To add to this madness, generate.sh took 24m 31.9s to run while I'm typing this whole thing out. I'm not using some awful commercial ISP - This is the internet at my university where I'm actually able to saturate the gigabit line rate for a lot of things, and npm is hosted on a CDN.

Now for the actionable part. "6000 lines is a too much code for a single package." is a valid concern, but we have had alternative approaches where we use fixed-output derivations and don't interfere with language-specific package managers. In the spirit of cargoSha256 for buildRustPackage, can we have npmSha256 for JS packages that use the npm ecosystem? For npm packages, package-lock.json includes not only the versions but also the content hashes for all dependencies. By default, npm install will automatically update the lock file if it does not match the developer-managed package.json, but there is npm ci which will simply fail the installation. Combined with --ignore-scripts it should produce deterministic outputs usable for our purposes. I have created a simple proof-of-concept.

Okay, thank you for listening to my TED talk. Updated the PR to bump the package to 1.28.1 and re-generate the packages with generate.sh. I may have silently broke an indeterminable amount of packages, but this is what we do, right? 🤷‍♂️

@zhaofengli zhaofengli changed the title bgpalerter: init at 1.27.1 bgpalerter: init at 1.28.1 Jun 29, 2021
@zhaofengli zhaofengli mentioned this pull request Jun 30, 2021
11 tasks
@zhaofengli
Copy link
Member Author

Just created a draft PR at #128749 for the proposed fetcher.

@Mic92
Copy link
Member

Mic92 commented Jun 30, 2021

Don't get me wrong. node2nix sucks for many reasons (like the scalability you mentioned) but non of the alternatives where suitable for nixpkgs so far. Here is a issue regarding caching/parallel downloads: svanderburg/node2nix#181

@Mic92
Copy link
Member

Mic92 commented Jun 30, 2021

Maybe just skip node-packages.nix for now and only update the json...

@zhaofengli
Copy link
Member Author

zhaofengli commented Jun 30, 2021

Don't get me wrong. node2nix sucks for many reasons (like the scalability you mentioned) but non of the alternatives where suitable for nixpkgs so far. Here is a issue regarding caching/parallel downloads: svanderburg/node2nix#181

As I mentioned in #128749, the alternative that I proposed does not attempt to Nixify the dependencies or reimplement the fetching. It simply uses the regular language-specific package manager which happens to provide the reproducibility we need.

Maybe just skip node-packages.nix for now and only update the json...

Sure, I'll let the merger do the generation for now.

(from linked issue)

Is there a way to force newest versions instead of the first version in the outer layer, assuming that newer versions are better? That should make it deterministic as well.

It's naive assumptions like this that is making nodePackages unreliable (please, read my post above). I'm not even sure whether we need to get this in right now since I'll still be using the one in my overlay because the one here in Nixpkgs will break at any moment without notice.

@zhaofengli
Copy link
Member Author

Also pinging @adisbladis out of courtesy since they seem to be the person referred to in the HN comment who suggested the approach. Please read my comment as well as #128749.

@stale
Copy link

stale bot commented Jan 3, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 3, 2022
@zhaofengli
Copy link
Member Author

Will be folded into #128749.

@zhaofengli zhaofengli closed this Sep 2, 2022
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.

None yet

4 participants