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

quarto: use pandoc 3.5 #357687

Merged
merged 3 commits into from
Dec 11, 2024
Merged

quarto: use pandoc 3.5 #357687

merged 3 commits into from
Dec 11, 2024

Conversation

detroyejr
Copy link
Contributor

Following up after the work done in #349683, we're updating the version of pandoc to 3.5 outside of Haskell's normal release cycle in order to improve compatibility with quarto.

Things done

Built and tested the result against the example given by @shriv https://github.com/shriv/quarto-1-6-30-pdf-reprex to make sure 3.5 renders the PDF document correctly.

  • 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.

@emilazy
Copy link
Member

emilazy commented Nov 20, 2024

I assume haskell-packages.nix here is generated code? I would really prefer to avoid doing this in a way that involves copy‐pasting rather than just adding the extra Pandoc version to the Haskell package set (if possible). cc @NixOS/haskell

@maralorn
Copy link
Member

maralorn commented Nov 20, 2024

@detroyejr pandoc_3_5 is already part of the Haskell package set. You can just fix the build of that via

  pandoc-cli_3_5 = super.pandoc-cli_3_5.overrideScope (self: super: {
    ... overrides ...
  };

in the configuration-common.nix file in the haskell-modules folder.
If any of the overrides you need does not exist in hackage-packages.nix in the right version you can generate by adding that version constraint to extra-packages in our main.yaml. Then run maintainers/scripts/haskell/regenerate-hackage-packages.nix.

Then you can use haskellPackages.pandoc-cli_3_5 in quarto.

@detroyejr
Copy link
Contributor Author

detroyejr commented Nov 20, 2024

Yes, it's copied from the auto-generated hackage repo file. pandoc_3_5 exists in the set already but doesn't build because the inputs resolve to the default older versions instead of the latest versions that pandoc 3.5 needs. haskell-packages.nix overrides the default version where needed until pandoc-cli builds correctly. Biggest issue to work through was version requirements from other packages (e.g typst >= 1.0 < 2.0 required, etc) so just adding the new version to the package set didn't seem to be enough.

Happy to try something else if this isn't the best way forward.

Edit: I'll check if the suggestion from @maralorn works. I seem to recall having difficulty with it, but was maybe doing something wrong.

@maralorn
Copy link
Member

Feel free to reach out with any problems you encounter. I am confident we can find a solution.

@detroyejr
Copy link
Contributor Author

@maralorn thank you for taking the time to provide feedback. This is significantly cleaner. I've tested this on a couple of different projects and am happy with the results so I'll mark this PR as ready.

@detroyejr detroyejr marked this pull request as ready for review November 21, 2024 02:37
@emilazy
Copy link
Member

emilazy commented Nov 21, 2024

I think this should be split into at least two commits – one to get the Pandoc package working and the other to use it in quarto. Other than that it looks good to me if @maralorn thinks so.

@emilazy emilazy added the backport release-24.11 Backport PR automatically label Nov 21, 2024
@maralorn
Copy link
Member

maralorn commented Nov 21, 2024

Two remarks. Both are not blockers from my point of view, but you might wanna fix it. Besides that, if compiles ship it.

  1. I don’t see a reason why you need the pandoc-lua-engine_0_3_3 overrideScope standalone on the toplevel. The trick with overrideScope is that it works transitively so you can do all overrides within the pandoc-cli override.

  2. The previous version of this PR had a decent amount of code concerned with reducing the closure size of the resulting package. I don’t know, but since the pandoc-cli-_… doesn’t have that by default. I’d estimate the closure size to be about 4GB bigger. Required to fix this would be a combo of enableSeperatebinOutputs/justStaticExecutables + running remove-references-to in postInstall.

@detroyejr
Copy link
Contributor Author

detroyejr commented Nov 21, 2024

I've addressed those 2 points. Pandoc was around 5GB in total now down to about 300MB so that's definitely improved. If I have time, I'll revisit the overrideCabal since I'm probably doing some unnecessary stuff there.

@maralorn
Copy link
Member

Okay, this is a bit wild... But I don't mind.

@emilazy
Copy link
Member

emilazy commented Nov 21, 2024

Shouldn’t the Haskell stuff here be in the Haskell files like it is with our main Pandoc?

@detroyejr detroyejr force-pushed the quarto-pandoc branch 2 times, most recently from 76da74c to 0133877 Compare November 21, 2024 23:08
@detroyejr
Copy link
Contributor Author

Maybe still a little wild, but I think this is moving in the right direction.

@emilazy
Copy link
Member

emilazy commented Nov 21, 2024

Sorry to keep bikeshedding about this :) as I’m not too familiar with the Haskell packaging infrastructure, but I just realized that this stuff is in pkgs/by-name/pa/pandoc/package.nix rather than haskell-modules itself, for the main Pandoc.

To reduce code duplication, how does this sound?

  1. Add pandoc-cli ? haskellPackages.pandoc-cli to the argument list of the pandoc package; use that instead of its default and use pandoc-cli.scope.pandoc instead of haskellPackages.pandoc.

  2. Add pandoc_3_5 = callPackage ../by-name/pa/pandoc/package.nix { pandoc-cli = haskellPackages.pandoc-cli_3_5; }; to pkgs/top-level/all-packages.nix.

  3. Use pandoc_3_5 in quarto.

This avoids any copy‐pasting and keeps the concerns nicely separated.

@detroyejr detroyejr force-pushed the quarto-pandoc branch 2 times, most recently from f475491 to 74fea36 Compare November 22, 2024 00:22
@detroyejr
Copy link
Contributor Author

detroyejr commented Nov 22, 2024

Not a problem! Yeah that looks much better.

Edit: Maybe could use 3 commits now instead of 2 splitting out fixing the haskell build and adding pandoc_3_5 cli. Let me know if you'd prefer that.

@emilazy
Copy link
Member

emilazy commented Nov 22, 2024

I do like 3 commits, and I like @maralorn’s suggestion, but I’m otherwise very happy with this :)

@maralorn
Copy link
Member

Cool to see so much improvement in one review process. Lgtm

@detroyejr
Copy link
Contributor Author

Thanks for all your help!

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

I can't comment on the haskell-specific stuff but the diff generally LGTM.

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

I think we’ve bikeshedded this one enough for now and it seems like a sensible fix :) Thanks for this!

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 25, 2024
@Samasaur1
Copy link
Member

Any chance this could get merged? It looks like there's no outstanding work to do

@shriv
Copy link

shriv commented Dec 11, 2024

Yes please! I was about to ask the same this week. 😊

@Atemu Atemu merged commit 648124c into NixOS:master Dec 11, 2024
35 checks passed
@nix-backports
Copy link

nix-backports bot commented Dec 11, 2024

Successfully created backport PR for release-24.11:

@Atemu
Copy link
Member

Atemu commented Dec 11, 2024

@emilazy are you sure this should be backported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: haskell 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants