[WIP] sage: use proper overlays

[timokau]

Motivation for this change

Sage has lots of dependencies and integrates tightly with them. Some of them have to be (temporarily) pinned at older versions. Currently that is done by simply using <pkg>.override. But since there are so many dependencies, they are likely to depend on each other. That makes it necessary to "wire them up" using lots of inherit's (as seen in #43679).

Since packageOverrides was deprecated, I'm unsure how to solve this. Here I use the variant mentioned in the manual, but not used anywhere in nixpkgs (yet). That re-imports nixpkgs however:

sagepkgs = import nixpkgs.path {
    overlays = [ ... ];
}

Is this a terrible idea? Will this lead to even more memory consumption? Will this ignore all previous user-defined overlays? Is there a better way?

@grahamc @matthewbauer @7c6f434c @FRidh

Other than the change from simple .override to overlays nothing has changed. So there is no need to review the details here (although you're of course free to do so).

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Fits CONTRIBUTING.md.

---

[grahamc]

IMO we'd just continue using packageOverrides in this case. Re-importing nixpkgs is very ugly, and all the user overlays will be lost inside of the reimport, not to mention system and configuration settings.

[FRidh]

packageOverrides is to override the package set from outside of it. Overlays are considered the superior solution and it is thus replacing it.

In this case, what is needed is a new scope. All the sub package sets use this as well, see e.g. Qt5:

libsForQt59 = lib.makeScope qt59.newScope mkLibsForQt5;

In order to make it possible to modify some of the Sage-specific overrides, one could offer this package set and allow a self: super: {}. See e.g. the Python package set.

[timokau]

Re-importing nixpkgs is very ugly, and all the user overlays will be lost inside of the reimport, not to mention system and configuration settings.

Yeah that's what I feared. I wish it was as easy as it is with the python package set.

In this case, what is needed is a new scope.

Can you help me with that? Unfortunately it seems like that isn't documented anywhere. I would need a scope that has all of the regular nixpkgs, but with some overrides. All the scopes I'm aware of only have new packages, not the rest of nixpkgs.

In order to make it possible to modify some of the Sage-specific overrides

That's probably overkill. At best there shouldn't be any sage specific overrides at all, whenever possible I try to backport compatibility patches instead.

[FRidh]

@timokau see FRidh@c6a649a

The withDoc part will need to be fixed though.

[FRidh]

Might as well finish it as well:
FRidh@c6e27de

Please test.

[timokau]

Thanks! Thats a lot to read through, I'll do that and test it tomorrow.

[timokau]

Unfortunately that does break the build:

Error compiling Cython file:
------------------------------------------------------------
...
from sage.libs.gmp.mpz cimport *
from sage.libs.flint.fmpz_poly cimport *
from sage.libs.flint.nmod_poly cimport *
from sage.libs.flint.ulong_extras cimport *

from cypari2.paridecl cimport (GEN, cgetg, t_POL, set_gel, gel, stoi, lg,
^
------------------------------------------------------------

sage/schemes/elliptic_curves/descent_two_isogeny.pyx:33:0: 'cypari2/paridecl/hyperellratpoints.pxd' not found

I'll figure out the details when I have more time.

Regarding the changes in general:

lib.fix' (lib.extends overrides (lib.extends packages (self: pkgs // {callPackage = pkgs.newScope self;})))

What does that do? I know what lib.fix is supposed to do, but I don't know about the rest.

sagePackages = callPackage ../applications/science/math/sage { };

Will this show up in search results?

[FRidh]

Unfortunately that does break the build:

I think there is something not OK with the callPackage here, or we have to explicitly pass in things with self.

lib.fix' (lib.extends overrides (lib.extends packages (self: pkgs // {callPackage = pkgs.newScope self;})))

Create a new package set that extends pkgs with a callPackage that looks into the current self when passing arguments to function calls:

self: pkgs // {callPackage = pkgs.newScope self;}

Take our "overlay" of modifications and extend our new package set with it:

lib.extends packages (self: pkgs // {callPackage = pkgs.newScope self;})

Extend once more with overrides "overlay" and then fix the final set:

lib.fix' (lib.extends overrides (lib.extends packages (self: pkgs // {callPackage = pkgs.newScope self;})))

[FRidh]

Will this show up in search results?

No, we don't recurse into that attribute set so it won't show up in say nix-env -q or nix search. But, you can access the attributes.

[FRidh]

The issue is that cypari2 doesn't use our version of pari. So, something is not yet correct with this new method.

[FRidh]

This should do the trick, although I'm not sure whether that can go in as I expect it to take up some memory.

[timokau]

Create a new package set that extends pkgs with a callPackage that looks into the current self when passing arguments to function calls:

Thanks for your explanation. The comments in lib/fixed-points.nix are pretty helpful too.

No, we don't recurse into that attribute set so it won't show up in say nix-env -q or nix search. But, you can access the attributes.

That's perfect.

This should do the trick, although I'm not sure whether that can go in as I expect it to take up some memory.

Okay, lets wait for @peti's answer.

That still doesn't quite build, but thats a different problem. We have to explicitly pass the nixpkgs version of pynac to sagelib again to keep it from using the python version. Then it works (without any rebuild even, proving that it is equivalent to my re-import overlay solution).

Should those overlaps in namespaces be considered an issue and fixed? They lead to pretty subtle bugs...

---

Also apart from the "overlay" changes I see that you're splitting the python and non-python parts further apart. For example moving python package to its own file (making it necessary to inherit them again) and using python.pkgs instead of passing them as arguments like regular pkgs.

Are there reasons for this? Wouldn't it be nicer to treat python pkgs and "regular" pkgs as uniformly as possible?

[timokau]

Also if fix -> fix' isn't acceptable, can't we use the same mechanism overlays use to make it possible to do deep overrides from within nixpkgs? Its not a unique sage problem, sage just needs it at a bigger scale than most packages. pythonPackages offers it, why not nixpkgs?

[FRidh]

Should those overlaps in namespaces be considered an issue and fixed? They lead to pretty subtle bugs...

They're annoying, but package names eventually will overlap. That's one reason why we use namespaces.

Are there reasons for this? Wouldn't it be nicer to treat python pkgs and "regular" pkgs as uniformly as possible?

Python libraries belong in the Python package set. That way we're also preventing possible naming collisions.

Also if fix -> fix' isn't acceptable, can't we use the same mechanism overlays use to make it possible to do deep overrides from within nixpkgs? Its not a unique sage problem, sage just needs it at a bigger scale than most packages. pythonPackages offers it, why not nixpkgs?

We are using the same mechanism: the package set we've defined here is exactly the same. The issue is that we need to re-evaluate the main package set. In order to do that, we need to access it, and there's two ways: the method you originally proposed, and attaching it to the evaluated package set as __unfix__ with fix'.

[timokau]

> Should those overlaps in namespaces be considered an issue and fixed? They lead to pretty subtle bugs... They're annoying, but package names eventually will overlap. That's one reason why we use namespaces.

That's true. Maybe the python (and similar) infrastructure's `callPackage` should refuse to pick a package if there are multiple options and force the user to specify one explicitly. No idea if that's technically feasible though.

> Are there reasons for this? Wouldn't it be nicer to treat python pkgs and "regular" pkgs as uniformly as possible? Python libraries belong in the Python package set. That way we're also preventing possible naming collisions.

But the name collision still happened this way. Probably I'm just idealizing in my thinking that implementation language should be an implementation detail :)

> Also if `fix -> fix'` isn't acceptable, can't we use the same mechanism overlays use to make it possible to do deep overrides from within nixpkgs? Its not a unique sage problem, sage just needs it at a bigger scale than most packages. `pythonPackages` offers it, why not `nixpkgs`? We are using the same mechanism: the package set we've defined here is exactly the same. The issue is that we need to re-evaluate the main package set. In order to do that, we need to access it, and there's two ways: the method you originally proposed, and attaching it to the evaluated package set as `__unfix__` with `fix'`.

That makes sense, I didn't consider that the original proposal just "fixed" this issue by re-importing.

[mmahut]

Are there any updates on this pull request, please?

[timokau]

Still blocked on something like #44196.

[stale]

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

1. Search for maintainers and people that previously touched the
   related code and @ mention them in a comment.
2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
   irc.freenode.net.

[stale]

I marked this as stale due to inactivity. → More info

[adamcstephens]

@collares It would be great to put some effort into this if you'd like to keep it open. It hasn't been touched in five years, was marked draft four years ago, has merge conflicts, and the PR linked above as a dependency was never accepted. Just keeping it open in its current state provides zero value.

[collares]

Fine by me in this particular instance, since I can reopen it if I find time in the future to work on this refactoring.

That being said, I don't think it's useful in general to close still-relevant issues/draft PRs even if they're "old", since the main thing this accomplishes is discarding progress/experience other people have gathered in the past, which future people may use in solving the problem. I don't think the prerequisite PR was rejected in any meaningful way; it just stalled and was closed for the same reason this PR is being closed, whatever that is (Keeping the number of open PRs low, I assume? Draft PRs should be easily filterable from queries, so tha's the only reason I can think of). I hope the CADT model doesn't become Nixpkgs standard policy.

[adamcstephens]

The PR content is maintained just as well in the closed state on github. Nothing is lost. WIP and Draft indicate someone is actively working on this issue, but that's clearly not the case, so I argue Closed is a much better indicator of the state of this PR. Future people will still be able to see it just the same. Though at this point this PR seems much closer to never being done than getting merged.

I'm not sure what the CADT model is, so I can't comment on that.

[collares]

That's fair. The discoverability aspect for closed PRs is a problem, so I filed issue #340754 as a compromise.

[adamcstephens]

An issue is a great idea. To be clear, I fully encourage someone to work on this if it's a real problem. As Anderson said when closing the other PR, it likely benefits to be done from scratch anyway.

The discoverability aspect for closed PRs is a problem

Out of curiosity, why do you find this the case? How is a draft more discoverable?

[7c6f434c]

Default filter includes drafts, excludes closed PRs (merged or not), and usually leaves a mostly acceptable number of PRs to skim.