search

Mic92 / nixpkgs-review

Review pull-requests on https://github.com/NixOS/nixpkgs
MIT License
401 stars 66 forks source link

Skipping packages does not actually skip building them #281

Open Atemu opened 2 years ago

Atemu commented 2 years ago 
nixpkgs-review pr 193589 --skip-package-regex tengine --skip-package-regex tensorflow

However, nixpkgs-review still tries to build tensorflow and tengine eventhough I explicitly declared to skip them:

acme.sh asn bind bind bind bitcoin-abc bitcoind-abc dgraph inxi jemalloc lean2 netdata nmapsi4 openvdb python3.10-baselines python3.10-distrax python3.10-edward python3.10-flax python3.10-gpt python3.10-optuna python3.10-pot python3.10-scikit-tda python3.10-tensorflow python3.10-tensorflow python3.10-tensorflow python3.10-tensorflow_probability python3.10-tflearn python3.10-trfl python3.10-umap-learn python3.10-vqgan-jax-unstable python3.9-baselines python3.9-distrax python3.9-edward python3.9-flax python3.9-gpt python3.9-optuna python3.9-pot python3.9-scikit-tda python3.9-tensorflow python3.9-tensorflow python3.9-tensorflow python3.9-tensorflow_probability python3.9-tflearn python3.9-trfl python3.9-umap-learn python3.9-vqgan-jax-unstable tengine tensorflow tensorflow testssl.sh twa

What I suspect is happening is that the skips only apply to direct dependencies rather than transitive dependencies. Even if this is intended, it breaks the principle of least surprise and should at least be documented in the help screen.

As a reviewer, I need the ability to completely disallow building certain packages to keep build times under control.

Mic92 commented 2 years ago

I have not found a way to get indirect dependencies cheaply from the nix-env query output so far. I would like to have the dependency tree for many reason though.

Atemu commented 1 year ago

An idea I had on this:

  1. Get list of attrs to build
  2. Get list of skip regex matches in that list
  3. Build an overlay where those attrs throw
  4. Filter out packages that throw in build.nix using (tryEval (toString package).success

It's not a perfect solution but a package that is depended upon by other modified packages should usually exist in the list of modified packages too.

Atemu commented 1 year ago

Would you accept a on this @Mic92?

doronbehar commented 3 months ago

BTW regarding the dependency tree, nix-output-monitor has logic in it that somehow parses the dependency tree and figures out how to print a dependency... Perhaps some ideas could be brought from there. It'd be also useful information to put in the result.md - showing which dependent packages failed due to a dependency failing, is very useful information.

Mic92 commented 3 months ago

Would you accept a on this @Mic92?

Marking packages as broken would be fine. Our report.md would get skewed a bit, but this is the ase anyway when only building a subset.

Mic92 commented 3 months ago

BTW regarding the dependency tree, nix-output-monitor has logic in it that somehow parses the dependency tree and figures out how to print a dependency... Perhaps some ideas could be brought from there. It'd be also useful information to put in the result.md - showing which dependent packages failed due to a dependency failing, is very useful information.

I parses the derivation in question, this would need to be done recursively.