Skip to content

treewide/pkgs: remove with lib;, part 2#335232

Merged
philiptaron merged 123 commits intoNixOS:masterfrom
philiptaron:issue-208242/follow-up-on-pr-334470
Aug 18, 2024
Merged

treewide/pkgs: remove with lib;, part 2#335232
philiptaron merged 123 commits intoNixOS:masterfrom
philiptaron:issue-208242/follow-up-on-pr-334470

Conversation

@philiptaron
Copy link
Contributor

Motivation for change

Description of changes

Follow-on to this PR:

meta = with lib; was only mutated if it contained non-regular statements, like if statements or // or the like.

I validated this change by comparing the outPath of all derivations that evaluated on master vs. the ones on this branch.

Things done

  • 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.

@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: printing Drivers, CUPS & Co. 6.topic: vim Advanced text editor 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: stdenv Standard environment 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: php PHP is a general-purpose scripting language geared towards web development. 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: dotnet Language: .NET labels Aug 16, 2024
@philiptaron philiptaron force-pushed the issue-208242/follow-up-on-pr-334470 branch from ce4fc55 to b63e155 Compare August 16, 2024 21:35
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 17, 2024
@philiptaron
Copy link
Contributor Author

https://gist.github.com/GrahamcOfBorg/1eab084267c1f1130592f05cc6e94380

huh

I'll take a look tomorrow at these and figure out why the rebuild.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nix nix-prefetch-git neovim-unwrapped nurl ]}" --prefix PYTHONPATH : "${./.}:${../../../../../maintainers/scripts}" )

This change causes a rebuild because it pulls in everything in the directory.

I think for an updater script, that's fine, if bad manners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pkgsLibPath = filter (pkgs.path + "/pkgs/pkgs-lib");
nixosPath = filter (pkgs.path + "/nixos");

This change causes a rebuild because lazy-options.json uses pkgs.path and thereby takes a reference on pkgs/pkgs-lib and nixos -- so any changes in those directories will cause a rebuild.

The actual output is identical.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems totally fine to me

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

I feel satisfied with the explanation for the rebuilds and like the change. The merge conflicts are probably going to be annoying. I suspect that some will prefer let inherit (lib) …; to the changes here, but it seems okay for those people to bikeshed that in their own packages later.

@philiptaron philiptaron marked this pull request as ready for review August 17, 2024 13:46
@philiptaron philiptaron force-pushed the issue-208242/follow-up-on-pr-334470 branch from b63e155 to 086aa4f Compare August 18, 2024 13:33
@philiptaron philiptaron merged commit 2b82213 into NixOS:master Aug 18, 2024
@philiptaron
Copy link
Contributor Author

Thank you all!

cudaPackages.cuda_cudart
cudaPackages.cuda_cccl # <thrust/*>
cudaPackages.libnpp # npp.h
cudaPackages.nvidia-optical-flow-sdk

Choose a reason for hiding this comment

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

@philiptaron nvidia-optical-flow-sdk is not contained in cudaPackages, this breaks opencv builds with cuda enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I messed this up. Fixed in #335880.

Copy link
Member

Choose a reason for hiding this comment

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

A good example of the pitfalls of with!

@philiptaron philiptaron deleted the issue-208242/follow-up-on-pr-334470 branch August 19, 2024 22:35
Comment on lines +48 to +50
inherit (lib) mkOptionType;
inherit (lib.types) nullOr oneOf coercedTo listOf nonEmptyListOf attrsOf either;
inherit (lib.types) bool int float str path;
Copy link
Contributor

Choose a reason for hiding this comment

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

@philiptaron was it intentional that these are inherited into the public/final pkgs.formats attrset?

I assume these should've been inherited into a let in block instead of directly into the rec { attrset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should have been in a let / in block. Will gladly merge that fix PR.

MattSturgeon added a commit to MattSturgeon/nixpkgs that referenced this pull request Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: dotnet Language: .NET 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 6.topic: php PHP is a general-purpose scripting language geared towards web development. 6.topic: printing Drivers, CUPS & Co. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: vim Advanced text editor 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants