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

typst: add initial support for typst packages #369283

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

Conversation

cherrypiejam
Copy link

@cherrypiejam cherrypiejam commented Dec 30, 2024

This PR adds initial support to build Typst documents with Typst packages. In particular, it adds a Typst package builder and exposes it at the top level to ease the process of defining new Typst packages. It further adds a new function to the Typst derivation for creating a new Typst environment in which the Typst compiler understands a specific set of packages, namely typst.withPackages. Moreover, three Typst packages are defined and collected as a single attribution set as an initial demonstration.

Only a few Typst packages are included in this PR for a reason. The Typst Universe (the official site for Typst packages) maintains a single git repo that contains all Typst packages. Each package entry does not necessarily map to the same version specified in their respective git repo published by their authors (if there is). This is because the package authors need to manually move their package and copy the source, instead of a pointer, to the Typst Universe repo. Breaking the Typst Universe repo down to per package requires separate places to store each of them, and due to the aforementioned reason, the exact source may disagree with the original git repo 1. This is why, in my opinion, we might want to collect our own set of Typst packages instead of directly pulling from the Typst Universe repo. For gradual adaption, typst.withPackages also provides a method to override the predefined set of Typst packages, namely typst.withPackages.override, making it easier for end users overriding the existing set of Typst packages and integrating new packages into their project.

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.

Footnotes

  1. Assuming the author(s) also publish their package in another repo outside of the Typst Universe repo.

@nix-owners nix-owners bot requested a review from philiptaron December 30, 2024 02:28
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Dec 30, 2024
@github-actions github-actions 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 Dec 30, 2024
@cherrypiejam cherrypiejam force-pushed the typst branch 3 times, most recently from b5ab425 to 0bcf7c9 Compare December 30, 2024 12:20
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Welcome to Nixpkgs! This is truly amazing; I've wanted to build Typst packages for a very long time but never got around to it.

Note that the commits should be split as much as possible. Each new typstPackage should be added in a separate commit. The changes to the base typst package to wrap it should also be a separate commit.

pkgs/by-name/ty/typst/with-packages.nix Outdated Show resolved Hide resolved
pkgs/build-support/build-typst-package.nix Outdated Show resolved Hide resolved
pkgs/development/typst-packages/cetz.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Regarding the actual package set, even though we can't import from Universe directly, it might still be a good idea to scrape it for information and build packages automatically. typst.toml gives us all of the necessary information to build each package.

pkgs/build-support/build-typst-package.nix Outdated Show resolved Hide resolved
pkgs/build-support/build-typst-package.nix Outdated Show resolved Hide resolved
pkgs/build-support/build-typst-package.nix Outdated Show resolved Hide resolved
pkgs/build-support/build-typst-package.nix Outdated Show resolved Hide resolved
pkgs/development/typst-packages/polylux.nix Outdated Show resolved Hide resolved
pkgs/development/typst-packages/polylux.nix Outdated Show resolved Hide resolved
pkgs/development/typst-packages/oxifmt.nix Outdated Show resolved Hide resolved
pkgs/development/typst-packages/cetz.nix Outdated Show resolved Hide resolved
pkgs/development/typst-packages/cetz.nix Outdated Show resolved Hide resolved
pkgs/by-name/ty/typst/with-packages.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hello,

Thanks for tackling this, I have a few comments, let me know if you have questions.

@jtojnar : I know you're busy these days, but if you could have a look at this, it would be great.

Thanks!

pkgs/build-support/build-typst-package.nix Outdated Show resolved Hide resolved
pkgs/build-support/build-typst-package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ty/typst/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ty/typst/with-packages.nix Outdated Show resolved Hide resolved
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 4, 2025
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 5, 2025
@cherrypiejam
Copy link
Author

Regarding the actual package set, even though we can't import from Universe directly, it might still be a good idea to scrape it for information and build packages automatically. typst.toml gives us all of the necessary information to build each package.

Ah that is great. Didn't know they already have that information included. I'll work on the script when I got time, but it will have to be waited for another week or so.

Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one final open question for anyone who knows about packagesFromDirectoryRecursive.

Comment on lines 15558 to 15560
typstPackages = callPackage ./typst-packages.nix {
callPackage = lib.callPackageWith (pkgs // { inherit buildTypstPackage; });
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
typstPackages = callPackage ./typst-packages.nix {
callPackage = lib.callPackageWith (pkgs // { inherit buildTypstPackage; });
};
typstPackages = lib.packagesFromDirectoryRecursive {
inherit callPackage;
directory = ../development/typst-packages;
}

Is this a good idea? It'd remove the need for typst-packages.nix, but I haven't seen packagesFromDirectoryRecursive in Nixpkgs before. (This is more of an open discussion and less of a review, though)

Copy link
Author

Choose a reason for hiding this comment

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

Have never used it before and happy to hear comments regarding it from other people

Copy link
Contributor

Choose a reason for hiding this comment

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

but I haven't seen packagesFromDirectoryRecursive in Nixpkgs before.

This is the mechanism in place for automatically discovering packages in pkgs/by-name.

Copy link
Author

Choose a reason for hiding this comment

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

Then I suppose we don't need it? typst-packages.nix is an expression defined in the top-level and itself is not a discoverable "package" (as it does not follow the naming convention <package-name/package.nix>). Alternatively we could rename it to a different path but I leave it here because all other *-packages.nix are defined in the same directory.

Also I was previously wondering how pkgs/by-name is discovered and joined to the pkgs set, and I guess this is it

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I suppose we don't need it?

It's up to you to decide how you want this to work. To be honest, I still don't understand everything in your PR so, I let you do some progress and then I'll test it out.

FYI: You can see an example of external use of packagesFromDirectoryRecursive in https://github.com/drupol/pkgs-by-name-for-flake-parts/

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I think not as many other *-packages.nix are defined directly in the top-level directory and I decide to follow this pattern. The just pushed change removes callPackage = lib.callPackageWith (pkgs // { inherit buildTypstPackage; }); as it is not needed.


preInstall = ''
rm -rf *
cp -r $BUILD_DIR/cetz/0.2.2/* .
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the feeling that this is overly complex. Can't we simplify this?

Copy link
Author

Choose a reason for hiding this comment

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

Essentially we need to generate the typst package, and remove non-essential parts (e.g, cetz building scripts). Feels like there are many ways of implementing it but they are similar in principle

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case I would avoid repeating the version more than once so it can be handled by rryan-tm bot at some point.

Copy link
Member

@jtojnar jtojnar Jan 6, 2025

Choose a reason for hiding this comment

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

Looking at https://github.com/cetz-package/common/blob/26359dfa24a9a24b71d38e9bb856b670e71a9118/scripts/package#L84-L84 and https://github.com/typst/packages?tab=readme-ov-file#local-packages, maybe just use

installPhase = ''
  runHook preInstall

  just install "$out/share/typst/packages/local/"

  runHook postInstall
'';

I do not see much of a point in buildTypstPackage in the first place.

Copy link
Author

Choose a reason for hiding this comment

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

There are a couple of things

  1. Not all typst packages need to be generated from their source. If the package source is the typst package, they only need to be moved to the right location where typst-env assumes. buildTypstPackage, IMO, eases this process. In other words, trivial typst packages do not need to write their own install phase and understand the assumption that typst-env makes.
  2. We want to seamlessly integrate nix into the compiling process of a Typst document. A Typst document can rely on two types of packages: one from the Typst universe (preview), and another is customized package (local). The Typst compiler, by design, understands packages from the Typst universe (it knows where to fetch the preview packages as long as if it has access to the Internet). As for local packages, Typst compiler shouldn't, and cannot, understand where to fetch, so it relies on the user to provide the source of local packages in a known place for the compiler to fetch. With that being said, when building Typst documents in nix, the Typst compiler should know the preview packages but cannot know since it doesn't have the access to the network (there's an alternative approach, will mention later). This is why we want to install the package to the preview directory to give Typst compiler this part of knowledge before the restricted (no network) build process. For the local packages, as mentioned earlier, the Typst compiler doesn't have the knowledge of where to fetch the source of local packages. This implies that the user must know where to fetch or what is the source. In other words, it is reasonable to leave the installation of local packages purely to the user.

This sounds like it might break ad-hoc package downloads https://github.com/typst/packages?tab=readme-ov-file#downloads

I believe my second point has answered this. Please let me know where doesn't make sense to you. This is all written in rush, so sorry in advance. Also, an alternative approach is to let Nix automatically fetch packages dependency of the target Typst document and wrap them into a derivation (e.g., Rust cargo builder, or Scala SBT builder). Thing is, the Typst compiler doesn't have a lock file (as the one used by the cargo builder for Rust), meaning that it is going to face the same issue of the SBT builder --- building twice and use the first build to retrieve the hash of all dependent packages. Alternatively, declaratively writing down required packages (the approach used in this PR) has a different issue --- nested dependency requires users to define all required packages in a single location, which can be extremely troublesome. To fix it, which is not yet to be implemented, we need to propagate dependencies from packages to the typst-env, which should be easy to implement.

Copy link
Author

Choose a reason for hiding this comment

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

In that case I would avoid repeating the version more than once so it can be handled by rryan-tm bot at some point.

Makes sense. Replaced with the version variable instead

pkgs/top-level/typst-packages.nix Outdated Show resolved Hide resolved
@drupol
Copy link
Contributor

drupol commented Jan 6, 2025

Would it be a good idea to have some documentation for this ?

@cherrypiejam
Copy link
Author

Would it be a good idea to have some documentation for this ?

I can write it, but where should I put it?

(Module updates) Added a release notes entry if the change is significant

Not sure if this change should be considered as significant

cp -r ${typst}/share $out/share
mkdir -p $out/bin
makeWrapper "${lib.getExe typst}" "$out/bin/typst" --set TYPST_PACKAGE_CACHE_PATH $TYPST_LIB_DIR
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like it might break ad-hoc package downloads https://github.com/typst/packages?tab=readme-ov-file#downloads

Copy link
Author

Choose a reason for hiding this comment

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

Please see my comment above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants