Skip packages that require manual downloading files

[piegamesde]

Describe the bug

nixpkgs-review is a great tool, but it suffers from quite a lot of false positives. One reason for them is obviously broken packages not marked as broken, this is already tracked in #85. The other reason, which I am concerned about, is packages that build (and thus are not marked as broken), but require some manual intervention in the build process. This may be by:

• Providing a license key (for nonfree packages)
• Providing a binary
• Manually doing some other things.

Steps To Reproduce

See NixOS/nixpkgs#91790. Of the 24 packages that fail to build, 8 failed because of the problem mentioned above. See citrix_workspace, worldofgoo, sqldeveloper and quartus-prime-lite as some examples.

Expected behavior

nixpkgs-review should recognize these packages and skip them, as it does for broken packages.

[Mic92]

The issue is, what if someone wants to actually test the package? Maybe it should rather mark them as broken due to requireFile if the file is not available, but I am not so sure how I can test this property from nix eval.

[piegamesde]

I sorry I don't fully understand what you are saying. But it's good to know that they all use requireFile and that this causes the problem.

[Mic92]

We use this nix script to check each package and filter those out that are broken: https://github.com/Mic92/nixpkgs-review/blob/master/nixpkgs_review/nix/evalAttrs.nix However I am not sure how this check would detect if requireFile is used.

[piegamesde]

That file is higher kinded Nix magic for me :) Also where does the json come from?

We could override requireFile in an overlay to somehow gain control over this, but I don't really know how this could then be used to extract that information.

An alternative way would be to add a new attribute to metadata, making this information easily accessible.

[Mic92]

One could add a throw statement in the requireFile override, however this would just mark the package as broken. maybe some meta attribute within requireFile could help. One could get this meta attribute via pkgs.<packagename>.src.meta assuming that requireFile is used for src, which should be the case for most packages.

[piegamesde]

So if I understand it correctly there are (at least) three different solutions to this:

1. Make requireFile crash and change the wording of "broken" to "skipped". "N packages marked as broken and skipped" could become "N packages are marked as broken or use requireFile" or something.

2. Override requireFile to inject some meta attribute into the result and then look if that attribute is there

3. Make requireFile crash. If the tryEval crashes, retry but without the override to distinguish if requireFile was used or if broken = true was set.

[Mic92]

I would still try to build packages using requireFile since some people want to actually test those packages but mark them not as skipped instead of broken if .src failed to build due to requireFile.