Skip to content

texlive.withPackages: use mkDerivation (finalAttrs: { ... }) instead of buildEnv#312945

Open
xworld21 wants to merge 6 commits intoNixOS:masterfrom
xworld21:texlive-buildtexenv-mkderivation
Open

texlive.withPackages: use mkDerivation (finalAttrs: { ... }) instead of buildEnv#312945
xworld21 wants to merge 6 commits intoNixOS:masterfrom
xworld21:texlive-buildtexenv-mkderivation

Conversation

@xworld21
Copy link
Contributor

Description of changes

Refactor texlive.withPackages to use mkDerivation (finalAttrs: { ... }) instead of buildEnv.

The main advantage is having texliveSmall.overrideAttrs { withDocs = true; }, which will allow us to deprecate texlive.combine, at last. Further benefit is that the build follows traditional phases, so one can add custom behaviours by setting the relevant pre/post-hooks.

Cons: I had to vendor a copy of the buildEnv script.

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.05 Release Notes (or backporting 23.05 and 23.11 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: TeX Issues regarding texlive and TeX in general labels May 19, 2024
@xworld21 xworld21 requested a review from veprbl May 19, 2024 18:07
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels May 19, 2024
@veprbl veprbl added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 21, 2024
@veprbl
Copy link
Member

veprbl commented May 21, 2024

Any plan to de-duplicate with the buildEnv script?

@xworld21
Copy link
Contributor Author

Any plan to de-duplicate with the buildEnv script?

Am I allowed to use the one in build-support/build-env? At some point I got the notion that folders should be self contained, but maybe it's just a by-name rule. I wouldn't mind using the same script.

xworld21 added 2 commits May 25, 2024 12:33
…bine emulation

The special package ordering was useful to guarantee that
texlive.combine would not change evaluation during the transition to
texlive.withPackages, and it is not needed any more.
@xworld21 xworld21 force-pushed the texlive-buildtexenv-mkderivation branch from f5b84b0 to bd66702 Compare May 25, 2024 11:41
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label May 25, 2024
@xworld21
Copy link
Contributor Author

The last update (which prevents the overrides from affecting the formats) seems to fix the performance regression:
image
The previous run reported some extra 60s – it might have been a CI fluke, but it got me worried.

@veprbl
Copy link
Member

veprbl commented May 26, 2024

Any plan to de-duplicate with the buildEnv script?

Am I allowed to use the one in build-support/build-env? At some point I got the notion that folders should be self contained, but maybe it's just a by-name rule. I wouldn't mind using the same script.

I think, you are allowed. If we can get the script no by path but via an attribute, that would be better.

@veprbl
Copy link
Member

veprbl commented May 26, 2024

Or, perhaps, converting buildEnv to mkDerivation is an option?

@xworld21 xworld21 force-pushed the texlive-buildtexenv-mkderivation branch from bd66702 to e2463f8 Compare May 26, 2024 19:16
@xworld21
Copy link
Contributor Author

I think, you are allowed. If we can get the script no by path but via an attribute, that would be better.

I have done so (hacky, because buildEnv is a function, but it's not outrageous).

Or, perhaps, converting buildEnv to mkDerivation is an option?

It may be too distruptive. buildEnv is written on the assumption that there won't be any phases, at best a kind of build phase (just so you can run postBuild). The new buildTeXEnv is split into unpack, configure, build, install and it runs fixup. I'd say they are too different at the moment.

A better solution, at the next round of improvements maybe, is to use buildEnv to create an 'unwrapped' output, and let buildTeXEnv deal only with actual building (formats, bin/ wrappers, ConTeXt cache and the likes).

@xworld21
Copy link
Contributor Author

xworld21 commented May 28, 2024

@GrahamcOfBorg eval

(Checking if the 30s eval increase is repeatable.)

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
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 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants