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

lib.meta.platformMatch: expand to partial systems without parsed & change some packages to using meta.badPlatforms #348192

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

RossComputerGuy
Copy link
Member

Things done

Many packages use meta.broken and just check stdenv whether the package is not supported. However, we should be using meta.badPlatforms. The idea is this can make meta easier to serialize. I've adjusted lib.meta.platformMatch to handle a partial system without requiring parsed to be present. I've updated the documentation to try and note people shouldn't be using meta.broken for simple checks. Some adjustments were necessary to lib.systems.mkSystemFromSkeleton and lib.systems.inspect in order to handle the new behavior, however the existing behavior is still intact. Some packages have been switched from meta.broken to meta.badPlatforms to showcase this. Rebuild count should be 0.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 6.topic: lib The Nixpkgs function library labels Oct 13, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 14, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 10, 2024
@ofborg ofborg bot removed the 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin label Nov 22, 2024
Copy link
Contributor

@SomeoneSerge SomeoneSerge Nov 26, 2024

Choose a reason for hiding this comment

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

In reply to: #357177 (comment)

The list of Nix platform types on which the package is known not to be buildable.

How I read this it's underspecified: "known not to be buildable" as in "unsupported", or not buildable temporarily as in meta.broken? I was not aware of any other uses of the interpretation that you assign to badPlatforms in your PR, even though it's an obvious improvement over meta.broken and justified by how correlated the three attributes (broken, platforms, badPlatforms) are. If badPlatforms are to be used to denote "temporarily broken", meta.broken should be deprecated - correlated attributes are confusing. A change like this could (with extra steps) allow to get rid of ad hoc constructions like

brokenConditions = attrsets.filterAttrs (_: cond: cond) {
"CUDA and ROCm are mutually exclusive" = cudaSupport && rocmSupport;
"CUDA is not targeting Linux" = cudaSupport && !stdenv.hostPlatform.isLinux;
"Unsupported CUDA version" =
cudaSupport
&& !(builtins.elem cudaPackages.cudaMajorVersion [
"11"
"12"
]);
"MPI cudatoolkit does not match cudaPackages.cudatoolkit" =
MPISupport && cudaSupport && (mpi.cudatoolkit != cudaPackages.cudatoolkit);
# This used to be a deep package set comparison between cudaPackages and
# effectiveMagma.cudaPackages, making torch too strict in cudaPackages.
# In particular, this triggered warnings from cuda's `aliases.nix`
"Magma cudaPackages does not match cudaPackages" =
cudaSupport && (effectiveMagma.cudaPackages.cudaVersion != cudaPackages.cudaVersion);
"Rocm support is currently broken because `rocmPackages.hipblaslt` is unpackaged. (2024-06-09)" =
rocmSupport;
};
too

How does this PR relate to the "warning system" effort?

Copy link
Member

@emilazy emilazy Nov 26, 2024

Choose a reason for hiding this comment

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

The Nixpkgs manual already says we should use badPlatforms instead of broken = …platform conditional…. FWIW. I think it gives us a nice delineation: platforms is some approximation of what upstream supports, perhaps modulated by what we think we could theoretically support; badPlatforms are platforms that currently we know we do not in practice support for one reason or another. broken is an opaque boolean that tooling can’t show usefully, which makes it worse than badPlatforms for this purpose. It does mean that you can’t use badPlatforms to do a simple set subtraction on platforms, but we have other ways to do that and that use‐case is rarer, so it seems okay to me, as long as we can get everyone on the same page.

And things can be broken based on conditions other than the host platform, so broken still has to exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

How does this PR relate to the "warning system" effort?

This PR mainly just makes the badPlatform meta attribute less strict. This allows us to have more flexibility and doesn't require the hack of adding a parsed empty attribute set and doesn't require using the predicates in lib.system.

My goal is this PR serves as a starting point to transition over a lot of the broken uses to bad platforms.

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.

4 participants