Skip to content

Conversation

@pbsds
Copy link
Member

@pbsds pbsds commented Mar 11, 2025

#364362 (comment) wants eigenVersions, but thats a bit overkill. Hence this, which follows package-naming

all should be in cache

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 11, 2025
@pbsds pbsds changed the title eigen_3_4_0: init at 3.4.0 eigen_3_4_0: init at 3.4.0, migrate eigen{,2,_3_4_0} to by-name Mar 11, 2025
@pbsds pbsds requested a review from Wulfsta March 11, 2025 22:36
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a verbatim copy of by-name/ei/eigen/include-dir.patch?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Well then we might as well reference it by ../... but also I'd prefer the "more generic expression" approach

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to resort to ../.. reach around, as i find them to be brittle.

IMO the maintenance burden is far larger when two packages have to go though a common generic.nix as they diverge more and more. I trust the tarball compression to outdo any compression we do by hand trying to re-using code.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the maintenance burden is far larger when two packages have to go though a common generic.nix

Yes, because that requires making .override and .overrideAttrs actually work, which could be considered one of the main value propositions of Nixpkgs

Copy link
Contributor

@SomeoneSerge SomeoneSerge Mar 11, 2025

Choose a reason for hiding this comment

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

My thinking was that fetchpatch {} could just go behind optionals (versionAtMost finalAttrs.version "3.4.0") (where versionAtMost = flip versionAtLeast, unless I'm confusing myself) in the eigen3 expression and then we'd just callPackage twice or use overrideAttrs in all-packages.nix (or when defining eigenVersions if we were doing that)

Copy link
Member Author

Choose a reason for hiding this comment

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

to satisfy nixpkgs-vet we still have to add a file at pkgs/by-name/ei/eigen_3_4_0/package.nix rather than all-packages.nix, unless you want to expose eigen_3_4_0 in eigen.passthru?

Copy link
Contributor

Choose a reason for hiding this comment

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

to satisfy nixpkgs-vet

Is this about the by-name check? Can't we just ignore it since we're adding it there for a reason and not because we haven't heard of by-name?

Copy link
Member Author

@pbsds pbsds Apr 20, 2025

Choose a reason for hiding this comment

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

I prefer not giving myself the license to circumvent codified guidelines and CI checks.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one person. 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 12, 2025
@pbsds
Copy link
Member Author

pbsds commented Apr 20, 2025

I just rebased on top of #397641 which also benefits from eigen_3_4_0.

@pbsds pbsds force-pushed the migrate-eigen-1741728350 branch from b42e966 to c9bcec0 Compare April 20, 2025 01:08
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 20, 2025
@pbsbot
Copy link

pbsbot commented Apr 23, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 389101


x86_64-linux

✅ 1 package built:
  • eigen_3_4_0

@pbsds pbsds merged commit 3a05537 into NixOS:master Apr 23, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants