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.

Sign up for GitHub

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

Jump to bottom

tectonic, tectonic-unwrapped, texpresso: fix build & clean up #384706

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

tectonic, tectonic-unwrapped, texpresso: fix build & clean up #384706

wants to merge 9 commits into from
+75 −70

Conversation

bryango
Copy link
Member

@bryango bryango commented Feb 24, 2025 •
edited
Loading

Supersedes:

See #384610 for context.

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.

@bryango bryango mentioned this pull request Feb 24, 2025
Closed
11 tasks
@bryango bryango marked this pull request as draft February 24, 2025 09:05
@bryango
Copy link
Member Author

bryango commented Feb 24, 2025 •
edited
Loading

Weird, I don't have this eval error locally... Probably something from master that hasn't gone into my local branch. Let me see...

Update:

  • ok I see it's c2fa08b. Let me see how to deal with it...
  • ah it's not c2fa08b, the reason is that our overrides here breaks texpresso

@doronbehar
Copy link
Contributor

I'm working on that.

bryango and others added 3 commits February 24, 2025 19:01
@bryango
texpresso, texpresso.tectonic: simplify overrides
5c8c79a 
The previous implementation (overriding `buildRustPackage`) breaks
downstream `buildRustPackage.overrides` in subtle ways. It's thus better
to use `.overrideAttrs` explicitly and reconstruct `cargoDeps` when
needed.

This commit should cause no rebuild, but the underlying change is
necessary for an upcoming patch on tectonic itself.
@doronbehar @bryango
tectonic: cleanup Darwin specific apple_sdk usage
dd7d980
@bryango
tectonic, tectonic-unwrapped: fix linux build
852f560
@github-actions github-actions bot added the 6.topic: TeX Issues regarding texlive and TeX in general label Feb 24, 2025
@bryango
Copy link
Member Author

bryango commented Feb 24, 2025 •
edited
Loading

I rewrote parts of the texpresso.tectonic overrides and it seems to work now. texpresso should not be rebuilt by these changes; only the overriding logic is refactored.

P.S. I will postpone the re-format of texpresso/tectonic as it will obscure the patch.

doronbehar
doronbehar reviewed Feb 24, 2025
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

So is this ready? Or not well tested? Can we mark it as ready?

@bryango
Copy link
Member Author

bryango commented Feb 24, 2025 •
edited
Loading

So is this ready? Or not well tested? Can we mark it as ready?

I haven't tested the build yet (only tested eval); but if it builds fine (and tectonic.passthru.tests succeed) then it should be ready!

@doronbehar
Copy link
Contributor

Testing it now with nixpkgs-review.

@doronbehar
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 384706

x86_64-linux

✅ 3 packages built:
  • tectonic
  • tectonic-unwrapped
  • texpresso

@doronbehar
Copy link
Contributor

Also tested the tectonic.passthru.tests derivation to work.

@doronbehar
Copy link
Contributor

There are a few additional commits I'd like to push, and I'll fix the nixfmt CI reported issue in a few minutes.

@doronbehar
texpresso.tectonic: fix meta
573d840
@doronbehar
texpresso.tectonic: nixfmt
c69f479
@doronbehar
texpresso.tectonic: set version
7f75c0a
@doronbehar
texpresso: callPackage ./tectonic.nix from package expression
6aadbf5
@doronbehar
texpresso: move to pkgs/by-name
4a37551
@doronbehar
texpresso: modernize / standardize
3e07cb7 
Move src closer to version attribute, so diffs of version bumps will be
smaller and easier to read, and a version attribute changing without a
hash changing will be more apparent, in case that happens.
@doronbehar doronbehar marked this pull request as ready for review February 24, 2025 11:41
@nix-owners nix-owners bot requested a review from lluchs February 24, 2025 11:49
@bryango
Copy link
Member Author

bryango commented Feb 24, 2025

I think it's good to go! Do we need to ping texpresso's maintainers? Or maybe we merge first since it's fixing broken builds?

@doronbehar
Copy link
Contributor

I think it's good to go! Do we need to ping texpresso's maintainers?

I wonder why CI hasn't pinged them already - there are many commits here prefixed with texpresso. But anyway, cc @NickHu .

@doronbehar
Copy link
Contributor

Let's give him say a day. Some of the changes here are somewhat stylistic and perhaps debatable.

@bryango
Copy link
Member Author

bryango commented Feb 24, 2025

I wonder why CI hasn't pinged them already - there are many commits here prefixed with texpresso.

It seems to me that the new github "nix-owners" bot is less reliable than the previous ofborg one. I have seen many cases recently that a maintainer is not automatically pinged... I have no idea why.

In any case, I have also tested .tex compile on an aarch64-darwin and all good.

@bryango bryango changed the title tectonic, tectonic-unwrapped: fix build & clean up tectonic, tectonic-unwrapped, texpresso: fix build & clean up Feb 24, 2025
@doronbehar
Copy link
Contributor

I suspect that it's due to the draft state that was modified during commits pushed, but I won't put an effort to verify & debug this.

@doronbehar doronbehar linked an issue Feb 24, 2025 that may be closed by this pull request
Build failure: tectonic #384793
Open
3 tasks
@doronbehar doronbehar mentioned this pull request Feb 24, 2025
Open
3 tasks
@MithicSpirit
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 384706

x86_64-linux

✅ 3 packages built:
  • tectonic
  • tectonic-unwrapped
  • texpresso

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doronbehar doronbehar doronbehar left review comments

@MithicSpirit MithicSpirit MithicSpirit approved these changes

@lluchs lluchs Awaiting requested review from lluchs

Assignees
No one assigned
Labels
6.topic: TeX Issues regarding texlive and TeX in general 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build failure: tectonic
3 participants
@bryango @doronbehar @MithicSpirit