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

texlive: create scope #250416

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

xworld21
Copy link
Contributor

@xworld21 xworld21 commented Aug 20, 2023

Description of changes

Third step along the progression multi-output / buildEnv / scope / ???: create a texlive scope that can be overridden in a monolithic way via overrideScope. Contains the minimum bits and pieces of #250626 to get here.

This should prove that the multi-output style and texlive.__buildEnv are complete enough to move forward.

Note that this is only groundwork for proper overridability: we need further bits, like an overridable buildTeXLivePackage, and a user-friendly way to override everything related to tlpdb; #250626 has more stuff about overriding the binaries in a nice way, allowing for different years to coexist, for instance.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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 the 6.topic: TeX Issues regarding texlive and TeX in general label Aug 20, 2023
@@ -478,12 +478,14 @@ let
texliveBinaries = bin;
};

tl = lib.mapAttrs (pname: { revision, extraRevision ? "", ... }@args:
texlivePackages = lib.makeScope newScope (self: lib.mapAttrs (pname: { revision, extraRevision ? "", ... }@args:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To understand: why do we need newScope here (as opposed to (x: x))? The only difference seems to be the attribute callPackage, which we do not use, after all. Everybody else seems to be using makeScope newScope.

Copy link
Contributor

@apfelkuchen6 apfelkuchen6 Aug 20, 2023

Choose a reason for hiding this comment

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

if we don't need/want callPackage, we could use

Suggested change
texlivePackages = lib.makeScope newScope (self: lib.mapAttrs (pname: { revision, extraRevision ? "", ... }@args:
texlivePackages = lib.makeExtensible (self: lib.mapAttrs (pname: { revision, extraRevision ? "", ... }@args:

In this case, the "extender"-attribute is called extend instead of overrideScope'.
Edit: there also is lib.makeExtensibleWithCustomName.

I don't really see why you would want to use id instead of newScope -- eval time should be identical due to the lazy nature of nix and using id will result in "broken" callPackage and newScope attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"broken" callPackage and newScope attributes.

My problem is probably that I don't quite understand what newScope is supposed to do. Is it to make it so the new callPackage knows all the attributes from the previous scope?

Edit: there also is lib.makeExtensibleWithCustomName.

Excellent. We want camel case attributes whenever possible to avoid a repeat of the combine clash, and makeScope injects packages, low risk name for sure, but still a risk. If we don't have a use for newScope and callPackage, it seems like an obvious choice.

@xworld21
Copy link
Contributor Author

@apfelkuchen6 I believe I jumped ahead of you!

@xworld21 xworld21 marked this pull request as draft August 20, 2023 20:27
@ofborg ofborg 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 labels Aug 20, 2023
@apfelkuchen6
Copy link
Contributor

The main benefit of the the approach in #120578 -- making the whole texlive attrset a scope -- is that it allows overriding the binaries, which isn't really possible with this approach, where just the package set is a scope.

Having overridable binaries is kind of neccessary for adding texlive-2023 without ugly case distinctions, so we will probably have to make texlive a scope anyway (I'm open to other ideas though). I'm not sure whether also making the package set a scope adds anything in this case. One could already override the package set with something along the lines of texlive.overrideScope (self: super: { pkgs = super.pkgs // { hyperref = super.pkgs.hyperref.override { ... }; }; }).combined.scheme-full while maintaining self-references correctly. When pkgs is also a scope nested in the other scope, one would have to use texlive.overrideScope (self: super { pkgs = super.pkgs.overrideScope' (pself: psuper: { hyperref = psuper.hyperref.override { ...}; }); }).combined.scheme-full or texlive.combine { inherit (texlive.pkgs.overrideScope' (pself: psuper: { hyperref = psuper.hyperref.override { ...}; });) scheme-full; }.

@xworld21
Copy link
Contributor Author

that it allows overriding the binaries, which isn't really possible with this approach, where just the package set is a scope.

I see. I suppose the key point here is that unlike lua/python/perl/etc, the binary part of texlive is not quite a monolithic package; where one would manipulate python3 directly at the top level, here we need to reach inside texlive. Even worse, some binaries have sources that come from the packages themselves! Like biber for example. Clearly if we override the package, the binary must change in sync, so all of texlive really must be a single scope.

TL;DR we have to move makeScope up one level so that the scope includes bin. The remaining challenge, aside from splicing, is that bin is nested, but it should be easy to rewrite it as a function of self (in practice, of self.bin), so that the following

texlive.overrideScope (final: prev: { bin = prev.bin // { core = prev.bin.core.overrideAttrs { ...}; }; })

achieves the intended result.

Does the above make sense? (Code tomorrow probably, if you don't beat me to it.)

@apfelkuchen6
Copy link
Contributor

That makes sense.

Unfortunately, getting the splicing to work in combination with the compatibility fixups that reinsert the packages into the toplevel is not particular easy. Just doing what everyone else does (defining the other splices "from the top" as pkgsBuildBuild.texlive etc (for example with generateSplicesForMkScope) results in infinite recursion. The idea I had to work around this is to explicitly define the other splices via pkgsBuildBuild.callPackage ./. {} and moving the compatibility fixups to another file -- with the drawback being that modifications to texlive via overlays don't propagate to the other splices.

Can you think of a better solution?

I have code for this lying around, I hope it is somewhat rebasable :D

@xworld21
Copy link
Contributor Author

I have code for this lying around, I hope it is somewhat rebasable :D

Yes please!

Any suggestions for debugging the splicing? E.g. how/where can I inspect __spliced?

@apfelkuchen6
Copy link
Contributor

Any suggestions for debugging the splicing? E.g. how/where can I inspect __spliced?

You can browse in the repl. Be aware that only cross package sets are spliced:

❯ nix repl --file .                                                
Welcome to Nix 2.17.0. Type :? for help.

Loading installable ''...
Added 20065 variables.
nix-repl> pkgsCross.aarch64-multiplatform.__splicedPackages.texlive.bin.core.__spliced
{ buildBuild = «derivation /nix/store/i28q1gbv0nn98qqq9zz1j13aqgdgwc3g-texlive-bin-2022.drv»; buildHost = «derivation /nix/store/i28q1gbv0nn98qqq9zz1j13aqgdgwc3g-texlive-bin-2022.drv»; buildTarget = «derivation /nix/store/i28q1gbv0nn98qqq9zz1j13aqgdgwc3g-texlive-bin-2022.drv»; hostHost = «derivation /nix/store/fr90pyv3nw1ngv3yaaycwa3bwdfnrg8c-texlive-bin-aarch64-unknown-linux-gnu-2022.drv»; hostTarget = «derivation /nix/store/fr90pyv3nw1ngv3yaaycwa3bwdfnrg8c-texlive-bin-aarch64-unknown-linux-gnu-2022.drv»; }

nix-repl> 

No idea how to debug infinite recursion though. I just noticed that currently cross-compilation of texlive.bin.core's dependency libavif is broken, so we cannot really test it anyway, but what I've done at least instantiates.

@xworld21
Copy link
Contributor Author

nix-repl> pkgsCross.aarch64-multiplatform.__splicedPackages.texlive.bin.core.__spliced

Oh so it's the packages under __splicedPackages that get the splices (well, most of them, not texlive.bin.core right now)... Thanks!

@apfelkuchen6
Copy link
Contributor

When accessing things via "fully qualified attr paths" such as pkgsCross.aarch64-multiplatform.__splicedPackages.texlive.bin.core.__spliced, everything should be spliced without any individual support by the specific derivations / scopes / whatever as it's handled from the top by splice.nix, which creates __slpicedPackages by recursing into all the attrs and adding the __spliced attributes to every derivation.

Adding custom splicing-magic is only neccessary when things in scope refer to each other "relatively" via the scope and not "fully qualified" via the top level. In this case, the splicing done in __splicedPackages is not applied yet, so the scope has to add the __spliced attributes for relative references itself.

But this means that there probably is no pretty way to debug whether the splicing is done correctly inside the scope, as we only have the "outside view" where splice.nix messes with the __spliced attributes (or does it? I don't fully understand all this splicing voodoo). So the only way to "correctly" debug the splicing in the scope is to check with which arguments the individual functions are actually called by inserting builtins.trace all over the place.

@xworld21
Copy link
Contributor Author

I think I have figured it out? I can create a scope using builtins tools only via makeScopeWithSplicing' and generateSplicesForMkScope "texlive". It seems to work as we want, because generateSplicesForMkScope recurses into attribute sets, so it reaches into bin.core as well. Then one needs to sprinkle self all over the place, and trust that overrideScope is doing its intended job, i.e. it applies the overrides to the splices.

Except thatgenerateSplicesForMkScope does not traverse lists, so our dear { pkgs = [ ... ]; } gets in the way. Again. But if we expose the packages as multi-output-like sets under texlive.pkgs, and make { pkgs = [ ... ]; } a compatibility shim, we should be fine in theory?

Give me a moment to complete the multi-output, then I'll rebase this PR, and we'll compare against your reference implementation.

@xworld21 xworld21 force-pushed the texlive-packages-scope branch from 528d0bc to a695286 Compare August 22, 2023 11:52
Comment on lines 575 to 587
texlive = makeScopeWithSplicing' {
f = makeTeXLive;
otherSplices = generateSplicesForMkScope "texlive";
};
in
# backward compatibility layer: expose packages directly under texlive, but not as derivations
# this cannot be done from within the scope
(builtins.mapAttrs
# flag if manpages are present
(n: p: let pWithMan = p // lib.optionalAttrs (p ? man) { texdoc = p.texdoc // { hasManpages = true; }; }; in {
pkgs = builtins.concatMap (n: lib.optional (p ? ${n}) p.${n}) [ "tex" "texdoc" "texsource" "tlpkg" "out" ];
})
texlive.__pkgs) // texlive
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compromise here is that you cannot use inherit (texlive.overrideScope ...) package, so this must come after buildEnv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xworld21 xworld21 changed the title texlive.pkgs: init texlive: create scope Aug 22, 2023
@xworld21 xworld21 force-pushed the texlive-packages-scope branch from a695286 to ffa7562 Compare August 22, 2023 12:00
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 and removed 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 labels Aug 22, 2023
Minimal implementation of texlive.buildEnv which builds the same outputs
as texlive.combine. Prefixed to allow for testing and changes to the
interface.
Initial step to convert the texlive attribute into a scope with slices.
This commit does not add the self references needed to make
.overrideScope useful.
@xworld21 xworld21 force-pushed the texlive-packages-scope branch from ffa7562 to 628ac6e Compare August 23, 2023 18:19
@@ -90,11 +90,12 @@ core = stdenv.mkDerivation rec {

nativeBuildInputs = [
pkg-config
] ++ lib.optionals (!stdenv.buildPlatform.canExecute stdenv.hostPlatform) [
] ++ lib.optionals (!stdenv.buildPlatform.canExecute stdenv.hostPlatform && self.bin.core ? __spliced) [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apfelkuchen6 this was causing infinite recursion when evaluating the splice hostHost, because in that case the build self.bin.core would be the same as the host self.bin.core, regardless of the true buildPlatform and hostPlaftorm. Conveniently, __spliced is not generated in hostHost, hence the ? __spliced guard.

I don't think I have seen this pattern anywhere. Is there a more idiomatic way to address the issue?

(This is an academic issue as we would never evaluate hostHost in the actual build, it was just annoying for debugging.)

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a bunch of derivations with cross self-dependenies (for example vala and guile), but I couldn't find anything that only adds itself if the derivation is spliced.

I'd consider it a non-issue. Maybe doing nothing about it is even better as it might detect actual problems at eval time.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 11-100 labels Aug 23, 2023
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: TeX Issues regarding texlive and TeX in general 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants