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

sile: 0.14.17 → 0.15.4 #323256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sile: 0.14.17 → 0.15.4 #323256

wants to merge 1 commit into from

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Jun 28, 2024

The upstream release is a big change from a pure Lua based CLI to Rust. The Rust CLI still includes a Lua VM that loads the rest of the system and the whole build is still autotools based, so most of the current derivation should apply including LuaRock dependencies. The only significant change is the addition of some Rust tooling.

See upstream release notes for v0.15.0, v0.15.1, v0.15.2, v0.15.3, and v0.15.4.

Most of this is copied from the project's Flake which has adapted to the changes during the development process. The one thing not reflected here yet is the abilitiy to change the Lua version by selecting a different package. I wasn't quite sure how to adapt that out of the flake into this derivation.

Copy link
Contributor

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

This will also need a rebase on current master, due to the change to meta.description

pkgs/tools/typesetting/sile/default.nix Outdated Show resolved Hide resolved
pkgs/tools/typesetting/sile/default.nix Show resolved Hide resolved
Comment on lines 67 to 71
# In Nixpkgs, we don't copy the Cargo.lock file from the repo to Nixpkgs'
# repo, but we inherit src, and specify a hash (it is a fixed output
# derivation). `nix-update` and `nixpkgs-update` should be able to catch that
# hash and update it as well when performing updates.
cargoDeps = rustPlatform.importCargoLock {
lockFile = ./Cargo.lock;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Ironically your code contradicts the comments and vendors Cargo.lock. But we don't need to vendor it here.

Suggested change
# In Nixpkgs, we don't copy the Cargo.lock file from the repo to Nixpkgs'
# repo, but we inherit src, and specify a hash (it is a fixed output
# derivation). `nix-update` and `nixpkgs-update` should be able to catch that
# hash and update it as well when performing updates.
cargoDeps = rustPlatform.importCargoLock {
lockFile = ./Cargo.lock;
};
cargoDeps = rustPlatform.fetchCargoTarball {
inherit (finalAttrs) src;
dontConfigure = true;
hash = "sha256-FD2otvk92/99AT3BEmfbID8j8MFIicshcocPZalPTHQ=";
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That comment came from code copied out of the upstream Flake, but I had to tinker with it so much I lost track of what it was saying. I was so confused by the apparent need to vendor this (I'm glad to find it is not actually needed).

Where did you get this hash?

Copy link
Contributor

Choose a reason for hiding this comment

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

I used the trust-on-first-use method, i.e. set hash = "";, build, and get the hash from the error. There might be a more elegant way that I don't know of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. Because I didn't understand it I tried using the hash of the Cargo.lock file from upstream, but that turned out not to work and gave me this hash in the error ;-) Given that the lock file is in the upstream source tarball and there is already a checksum on that, it seems like a bit of redundant work here. Whatever this is actually hashing should be derived from the already checksumed source inputs.

pkgs/tools/typesetting/sile/Cargo.lock Outdated Show resolved Hide resolved
, makeWrapper
, pkg-config
, poppler_utils
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused about all of those formatting and reorder changes that enlarge the diff (for so no real reason so it seems). I also noticed your other reformat PR upstream:

sile-typesetter/sile#2081

What is the reason behind all of this changes? Which expression should lead the formatting standard? BTW we have nowadays a consensus sort of regarding formatting, it is contained in the package nixfmt-rfc-style.

Whatever decision you'll go with, I'd suggest to apply a nixfmt + consistent argument reordering, and then perform the changes related to the update itself - it will make the PR a bit easier to review. Another option is to discard any of the formatting changes, and minimize the diff as much as possible, which is also legitimate.

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 re-ordering was because both the package here and the upstream flake had different orders making them hard to diff, and neither order made any sense to me. Re-ordering them both grouping by type and then alphabetically brought some rhyme and reason to them. I'll look into nixfmt.


src = fetchurl {
url = "https://github.com/sile-typesetter/sile/releases/download/v${finalAttrs.version}/sile-${finalAttrs.version}.tar.xz";
sha256 = "sha256-f4m+3s7au1FoJQrZ3YDAntKJyOiMPQ11bS0dku4GXgQ=";
url = "https://github.com/sile-typesetter/sile/releases/download/v${finalAttrs.version}/sile-${finalAttrs.version}.tar.zst";
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, why change to zst?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've always wanted to use ZST, the reason for XV in the first place was that the GitHub Actions runners we were using to build the source had a version of GNU Autotools that did not support the zstd extension out of the box. Using XZ was less resistance. With more recent LTS of runners available by default that is no longer a limitation, and the generated configure scripts work on older systems as well so there were no remaining reasons not to use ZST which has in my mind superior properties.

Then the whole snafu with XZ being backdoored happened and we had folks complaining about the use of XZ. The complaints don't have much merit since the compromise did not affect generated archives, but since I'd always wanted ZST anyway just switching seemed like the easy move. I switched a bunch of my projects based on that logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants