Skip to content

Conversation

@h0nIg
Copy link
Contributor

@h0nIg h0nIg commented Oct 18, 2025

#421125 introduced a problem with checkMeta=true for nixpkgs-review. checkMeta defaults to false in general.

The scenario happened for thunderbird-bin in combination with an unsupported platform.
This PR prevents any evaluation which might be problematic

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@h0nIg h0nIg force-pushed the patch-34 branch 2 times, most recently from 7cf7193 to 65be3b7 Compare October 18, 2025 18:44
@h0nIg h0nIg marked this pull request as ready for review October 18, 2025 18:45
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 6.topic: stdenv Standard environment labels Oct 18, 2025
@jopejoe1
Copy link
Member

This does not fix all cases that broke due to the change, it still breaks on cplex for example.

error:
       … while evaluating an expression to select 'drvPath' on it
         at «internal»:1:552:
       … while evaluating strict
         at «internal»:1:552:
       (stack trace truncated; use '--show-trace' to show the full trace)

       error: This nix expression requires that the cplex installer is already
       downloaded to your machine. Get it from IBM:
       https://www.ibm.com/support/pages/downloading-ibm-ilog-cplex-optimization-studio-2211

       Set `cplex.releasePath = /path/to/download;` in your
       ~/.config/nixpkgs/config.nix for `nix-*` commands, or
       `config.cplex.releasePath = /path/to/download;` in your
       `configuration.nix` for NixOS.

@h0nIg
Copy link
Contributor Author

h0nIg commented Oct 19, 2025

This does not fix all cases that broke due to the change, it still breaks on cplex for example.

error:
       … while evaluating an expression to select 'drvPath' on it
         at «internal»:1:552:
       … while evaluating strict
         at «internal»:1:552:
       (stack trace truncated; use '--show-trace' to show the full trace)

       error: This nix expression requires that the cplex installer is already
       downloaded to your machine. Get it from IBM:
       https://www.ibm.com/support/pages/downloading-ibm-ilog-cplex-optimization-studio-2211

       Set `cplex.releasePath = /path/to/download;` in your
       ~/.config/nixpkgs/config.nix for `nix-*` commands, or
       `config.cplex.releasePath = /path/to/download;` in your
       `configuration.nix` for NixOS.

fixed. even though it was advised in matrix that tryEval might not catch everything, it matches this exact case, so we should add it

nix-repl> legacyPackages.x86_64-linux.cplex.meta.identifiers
{
  cpeParts = { ... };
  possibleCPEs = [ ... ];
  purlParts = { ... };
  purls = [ ... ];
  v1 = { ... };
}

nix-repl> legacyPackages.x86_64-linux.cplex.meta
{
  available = true;
  broken = false;
  description = "Optimization solver for mathematical programming";
  homepage = "https://www.ibm.com/be-en/marketplace/ibm-ilog-cplex";
  identifiers = { ... };
  insecure = false;
  license = { ... };
  mainProgram = "cplex";
  maintainers = [ ... ];
  maintainersPosition = { ... };
  name = "cplex-22.11";
  outputsToInstall = [ ... ];
  platforms = [ ... ];
  position = "/nix/store/n5069qmzc6dfg8w1c6n0nzba60z3cai7-source/pkgs/applications/science/math/cplex/default.nix:175";
  sourceProvenance = [ ... ];
  unfree = true;
  unsupported = false;
}

@jopejoe1
Copy link
Member

Tested again and can confirm that this now works with everything that is currently listed on search.nixos.org

@wolfgangwalther
Copy link
Contributor

Unfortunately, I am lacking ideas how to fix this properly on the spot, thus I would like to proceed with the revert in #453322.

@h0nIg
Copy link
Contributor Author

h0nIg commented Oct 19, 2025

Unfortunately, I am lacking ideas how to fix this properly on the spot, thus I would like to proceed with the revert in #453322.

@wolfgangwalther a second approach was highlighted by @dramforever in matrix, this looked odd in the beginning, but maybe this is a better approach. would you feel more comfortable with this?

diff --git a/pkgs/stdenv/generic/check-meta.nix b/pkgs/stdenv/generic/check-meta.nix
index 3a35b750d5f3..f947ea181adc 100644
--- a/pkgs/stdenv/generic/check-meta.nix
+++ b/pkgs/stdenv/generic/check-meta.nix
@@ -444,7 +444,7 @@ let
       maintainersPosition = any;
       teamsPosition = any;
 
-      identifiers = attrs;
+      identifiers = any;
     };
 
   checkMetaAttr =

@wolfgangwalther
Copy link
Contributor

a second approach was highlighted by @dramforever in matrix, this looked odd in the beginning, but maybe this is a better approach. would you feel more comfortable with this?

This would still not allow me to convert all of meta to json for these packages, right?

@h0nIg
Copy link
Contributor Author

h0nIg commented Oct 19, 2025

a second approach was highlighted by @dramforever in matrix, this looked odd in the beginning, but maybe this is a better approach. would you feel more comfortable with this?

This would still not allow me to convert all of meta to json for these packages, right?

with the current approach in this PR you can json-ify

a second approach was highlighted by @dramforever in matrix, this looked odd in the beginning, but maybe this is a better approach. would you feel more comfortable with this?

This would still not allow me to convert all of meta to json for these packages, right?

no, you are right. but lets summarize to get an overview:

there are 2 cases we hit:

  1. tools like nixpkgs-review, which ignore certain meta informations and which are willingly accessing meta even if it might be broken. They need to enable checkMeta=true as well to run into issues, which is turned off by default
  2. tools which just want to get listed on search.nixos.org, like cplex, which do not provide a src and which are not buildable. They throw an exception, since the src is not available (needs override)

there are 6 options:

  1. go back to previous implementation 8797747 -1 commit, where we explicitly configure languages which are known to play by the rules. We lose general support with this
  2. merge this PR, which enables json-ify according to stdenv: pURL implementation - fix checkMeta #453291 (comment) and which fixes the exotic cases - but is "not so nice". It follows the principles of non-support / use "throw" according to (mostly) Pass nixpkgs release checks on RISC-V #420442
  3. go with the proposal stdenv: pURL implementation - fix checkMeta #453291 (comment) which does not enables json-ify
  4. revert
  5. merge thunderbird-bin: throw on unsupported system #453333 which fixes the symptom and lifts the blocker. We can decide about next steps tomorrow. It would give @RossComputerGuy and others the time to decide on next steps
  6. feature-toggle the inheritance from drv.src.meta to drv.meta. People using SBOM tools can enable this on purpose. People can still benefit once they find drv.src with their SBOM analysis tool (doing recursive drv analysis from outside of nixpkgs)

my favorite is 2 + 6. "throws" is commonly used on platforms where there is no support, e.g. similar to #420442 - tryEval catches this exact scenario

With this in place, we protect against further problems, still fulfill the request to have a general support

@h0nIg
Copy link
Contributor Author

h0nIg commented Oct 19, 2025

Tested again and can confirm that this now works with everything that is currently listed on search.nixos.org

@jopejoe1 can you elaborate how you tested this? this would create more confidence

@dramforever
Copy link
Contributor

  1. revert, because regressions always require immediate revert, regardless if they have major import or not?

It's not because regressions always require immediate revert, but because

  • Breaking local nixpkgs-review on aarch64-linux is definitely of major import
  • This pURL thing has introduced a dependency of meta on src, which is not desirable. The use cases broken are symptoms of this issue.

@jopejoe1
Copy link
Member

jopejoe1 commented Oct 19, 2025

Tested again and can confirm that this now works with everything that is currently listed on search.nixos.org

@jopejoe1 can you elaborate how you tested this? this would create more confidence

I build the release tarball with #451424 so that it checks all meta attrs of all packages included in the packages.json file and not silently ignores errors making it possible to check those.

@wolfgangwalther
Copy link
Contributor

  • This pURL thing has introduced a dependency of meta on src, which is not desirable. The use cases broken are symptoms of this issue.

Very much agree here.

@wolfgangwalther
Copy link
Contributor

Closing because #453322 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants