Zola: Add --locked flag to cargo install command, update sha256 checksum

[samford]

• Have you followed the guidelines for contributing?
• Have you checked that there aren't other open pull requests for the same formula update/change?
• Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
• Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
• Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

---

Building Zola from source currently fails since cargo install doesn't use the dependency versions specified in the Cargo.lock file by default. A dependency released a breaking change between the time Zola 0.9.0 was released and now, so the build now fails with errors. This commit adds the --locked flag to cargo install to ensure that the Cargo.lock file is used when building from source.

At least to me, it makes sense to use the --locked flag anyway, as it ensures that the build is predictable/reproducible and won't vary over time due to dependency updates. It looks like other formulas using cargo install don't use the --locked flag either, so this may be something to consider on a larger scale.

Past that, it's expected for the builds to fail here, since the sha256 checksum changed between release and the current tar.gz for Zola 0.9.0, as mentioned in #45819.

[SMillerDev]

Would you be willing to check all other homebrew formulae for this oversight?

[samford]

I looked through the other formulae and, of the ones that use cargo install, none of them use the --locked flag. The basis of the cargo install command that's used in these formulae comes from Homebrew/brew/Library/Homebrew/formula_creator.rb, which doesn't use the --locked flag.

I checked the source for each of the relevant formulae and compiled a table, listing if a Cargo.lock file is present.

Formula	Has Cargo.lock
angle-grinder.rb	Yes
badtouch.rb	Yes
bat.rb	Yes
boringtun.rb	Yes
broot.rb	Yes
click.rb	Yes
cmdshelf.rb	Yes
deno.rb	Yes
diffr.rb	Yes
diskus.rb	Yes
dssim.rb	No
dust.rb	Yes
fastmod.rb	Yes
fd.rb	Yes
ffsend.rb	Yes
fselect.rb	No
geckodriver.rb	Yes (shared)*
genact.rb	Yes
gifski.rb	Yes
git-absorb.rb	Yes
git-delta.rb	Yes
git-series.rb	Yes
gleam.rb	Yes
hexyl.rb	Yes
hyperfine.rb	Yes
interactive-rebase-tool.rb	Yes
just.rb	Yes
ktmpl.rb	Yes
loc.rb	Yes
lsd.rb	Yes
mdbook.rb	Yes
mdcat.rb	Yes
miniserve.rb	Yes
nushell.rb	Yes
oxipng.rb	Yes
pastel.rb	Yes
pijul.rb	Yes
pngquant.rb	No
procs.rb	Yes
rargs.rb	Yes
rbspy.rb	Yes
ripgrep-all.rb	Yes
ripgrep.rb	Yes
rust.rb	Yes/No**
rustup-init.rb	Yes
sccache.rb	Yes
sd.rb	Yes
shadowenv.rb	Yes
shellharden.rb	Yes, with next release
sk.rb	Yes
sn0int.rb	Yes
ssh-permit-a38.rb	Yes
starship.rb	Yes
svgcleaner.rb	No
tealdeer.rb	Yes
tectonic.rb	Yes
tokei.rb	Yes
topgrade.rb	Yes
wagyu.rb	Yes
watchexec.rb	Yes
websocat.rb	Yes
xsv.rb	Yes
zola.rb	Yes

* With geckodriver.rb, the Cargo.lock file is instead part of a shared workspace (lock file seen here) rather than being in the geckodriver folder, so the formula would seemingly need to download that shared Cargo.lock file somehow.

** With rust.rb, Rust and Racer each have a Cargo.lock file but Cargo does not.

For the ones with a Cargo.lock file, it should be safe to add the --locked flag to the cargo install command but it would be good to do test builds after the change to make sure they still build properly. I don't see any reason why the builds would fail (unless a release is broken or Cargo.lock wasn't updated for the release) but I like to err on the side of caution.

If you need me to go through these formulae and add the --locked flag to the relevant ones, how would you prefer this to be done?:

• A. One PR and one commit with all modified formulae
• B. One PR and separate commits for each modified formula
• C. Separate PRs and separate commits for each modified formula

Depending on the answer, I can close this PR if it's preferred to do these changes all in one PR.

Past that, we could create issues on the repos without a Cargo.lock file, asking them to add one and updating the formula if a Cargo.lock file is added. It's recommended in the Cargo book to check the Cargo.toml file into version control for any "executable like command-line tool and application", so it's reasonable to ask.

Lastly, I can also open an issue in the Homebrew/brew repo, asking if --locked should be in the default cargo install command. I think it maybe warrants a little discussion since a majority of Rust binary projects have a Cargo.lock file but clearly not everyone knows to check it into version control. I could also just create a PR modifying the applicable files and start the discussion that way, if it makes more sense.

[SMillerDev]

@Homebrew/maintainers please have a look here.

@samfordThanks for the detailed information. I have very little knowledge about rust but I hope one of the other maintainers does.

[reitermarkus]

If there is a Cargo.lock, we should definitely be using --locked for reproducibility.

we could create issues on the repos without a Cargo.lock file

Yes, binaries should always have the Cargo.lock in version control. Not sure how to handle crates with both a library and a binary though.

[samford]

I created Cargo.lock-related issues in the repos for: dssim, fselect, pngquant, and shellharden. The svgcleaner repo already had an old issue, so I left a comment and the creator responded saying, "The project is sort of dead. To bump a version I have to build cli/gui packages too. Which will take a lot of time and I don't have any right now."

If any of the other projects add a Cargo.lock file, I'll update the table in the previous comment.

Edit: The creator of shellharden added a Cargo.lock file but we've agreed that it doesn't make sense to publish a new release simply to include the file. When the next release is published, the Cargo.lock file will be present in the archive. [For what it's worth, shellharden currently doesn't have any dependencies.]

I took another pass through these formulae and created a list of projects that have both [[bin]] and [lib] sections in Cargo.toml:

• boringtun
• dssim
  • No Cargo.lock file but issue has been created
• gifski
• nushell
• oxipng
• rustup (rustup-init.rb)
• sk
• svgcleaner
  • No Cargo.lock file and likely won't see one anytime soon
• wagyu

Of this list, only the dssim and svgcleaner projects lack a Cargo.lock file, so it's definitely possible to handle both a binary and library in the same project while still having a Cargo.lock in version control. It's incumbent upon the creator (or maintainer(s)) to ensure that Cargo.lock is either included or excluded when publishing to crates.io, as appropriate.

cargo install will build and install the binary (not the library), so I don't think this is a concern for Homebrew. Have I missed anything?

[reitermarkus]

Have I missed anything?

I don't think so. Great work so far!

[MikeMcQuaid]

Thanks @samford, great work here. Do you think this would be worth a brew audit to flag when --locked is not being used?

[chrmoritz]

With geckodriver.rb, the Cargo.lock file is instead part of a shared workspace (lock file seen here) rather than being in the geckodriver folder, so the formula would seemingly need to download that shared Cargo.lock file somehow.

I think this will solve itself, because we need the full mozilla-central source code for any future geckodriver updates (including the current latest version not yet in homebrew) anyway, see #45217 (comment).

[samford]

Do you think this would be worth a brew audit to flag when --locked is not being used?

I'll leave this question up to you and the other Homebrew members, since I don't have enough domain knowledge of Homebrew yet to determine when it is or isn't appropriate to add an audit for something.

I ran a test and, at least on my system (macOS 10.15), brew install --build-from-source dssim ran and installed fine with the --locked flag added to the formula, even though dssim currently doesn't have a Cargo.lock file in its source. The Cargo Book is a little ambiguous about what happens when the --locked flag is used and a Cargo.lock file isn't present but it seems like it works the same way as the default cargo install command (using the latest dependencies), rather than giving an error and failing.

As such, it may be fine to always include the --locked flag by default, regardless of whether a Cargo.lock file is present in the source. From my limited understanding, adding the --locked flag to existing formulae as well as the boilerplate cargo install command in formula_creator.rb seems like it would take care of this issue for the most part but do let me know if I've overlooked anything.

Modifying the boilerplate cargo install command would appear to require changes to the following files in the Homebrew/brew repo: /Library/Homebrew/formula_creator.rb, /Library/Homebrew/rubocops/text.rb, and /Library/Homebrew/test/rubocops/text_spec.rb.

It would be good for folks to check my work here (running their own tests, identifying any other files that would need to be modified, etc.) and to weigh in on how best to proceed. If a consensus is reached on how to handle this and you need me to take care of any of the changes, just let me know.

[MikeMcQuaid]

I'll leave this question up to you and the other Homebrew members, since I don't have enough domain knowledge of Homebrew yet to determine when it is or isn't appropriate to add an audit for something.

As such, it may be fine to always include the --locked flag by default, regardless of whether a Cargo.lock file is present in the source.

Definitely sounds like an audit would be desirable then 👍

[chrmoritz]

@samford Are you sure that's the case?:

The Cargo Book is a little ambiguous about what happens when the --locked flag is used and a Cargo.lock file isn't present but it seems like it works the same way as the default cargo install command (using the latest dependencies), rather than giving an error and failing.

At least to documentation states:

If the lock file is missing, or it needs to be updated, Cargo will exit with an error.

However it will also look in parent directories for it.

[samford]

Are you sure that's the case?

If the lock file is missing, or it needs to be updated, Cargo will exit with an error.

I noticed that in the Cargo Book and fully expected Cargo to produce an error and exit when the --locked flag is used and a Cargo.lock file isn't present but I've yet to be able to produce any errors using recent stable versions (1.38.0, 1.37.0) or a recent nightly (1.40.0-nightly (7979016af 2019-10-20)).

I've tried this in and outside of Homebrew using projects with and without a Cargo.lock file (i.e., no Cargo.lock file anywhere in the source). This is primarily the reason why I was interested in having others test this as well because the documentation says one thing but something different may happen in practice. I don't want to paint this as a safe default when it could potentially error but I can't produce an error no matter how I try.

Regardless, it would be safer to simply take the Cargo documentation at its word. With that in mind, it could be better to not have --locked as the default for new formulae (e.g., in formula_creator.rb) and to rely on an audit to enforce having it added only when it's appropriate (if possible).

If an audit for the --locked flag is created, is it possible to write it such that it 1) checks if cargo install is used to build/install the project, 2) checks for a Cargo.lock file in the source, 3) checks if the --locked flag is missing from the cargo install command, and only produces an audit warning/error if all those conditions are true? I'm unfamiliar with how audits are typically written and what context they have available.

The part that may be tricky is accurately determining if a Cargo.lock file is present without producing false positives in more complex layouts (e.g., multiple projects in one repo). The overwhelming majority have a Cargo.lock in the root directory, even if there are projects in subdirectories that inherit the base Cargo.lock file. I don't think I've come across a project where Cargo.lock is in a subdirectory but it could potentially be the case where a repo has multiple projects side by side. In this case, the audit would seemingly need to know the correct folder to look in for it to work properly and I'm unsure of whether that's possible.

As mentioned, I'm rather naive about Homebrew's inner workings, so take all this with a grain of salt. I'm glad for the discussion around this, as I don't wish to be the arbiter of how this should work or be implemented and I'm merely putting forth observations and ideas to fuel the conversation.

[MikeMcQuaid]

If an audit for the --locked flag is created, is it possible to write it such that it 1) checks if cargo install is used to build/install the project, 2) checks for a Cargo.lock file in the source, 3) checks if the --locked flag is missing from the cargo install command, and only produces an audit warning/error if all those conditions are true? I'm unfamiliar with how audits are typically written and what context they have available.

No, unfortunately not. They can only evaluate the text of the formula or the post-install files but not the pre-install files/source.

I think we'd rather have a formula_creator and audit sensible default that works in the majority of cases and has the potential for a whitelist that can be used when needed.

[zbeekman]

Great discussions about audits and formula_creator!

I don't want to distract/detract from that, but in the interest of getting this formula update merged, could the other open Zola PR with sha256 update be combined with this change? (And check if there's a new release of zola, we can bump the version at the same time if there is.)

[Moisan]

There is no newer Zola release. Upstream talks about the checksum here, which was also mentioned in the first comment of this PR. Simply updating the checksum here should be fine I think.

[samford]

I'll wait to continue the general discussion about the --locked flag, in case this needs to move to an issue, etc. after the PR is closed.

...in the interest of getting this formula update merged, could the other open Zola PR with sha256 update be combined with this change? (And check if there's a new release of zola, we can bump the version at the same time if there is.)

Zola releases are a bit sporadic and I don't imagine there will be a new one for a little while, which is why I moved forward with these PRs (to fix the formula in the interim time). I'm working on a feature that could be notable enough to warrant releasing a new version but it will be at least a week before it could technically be published and merged (i.e., I'm waiting for the release of Rust 1.39.0 and certain crates finalizing their currently-alpha releases). Usually a release isn't published immediately after a notable change lands and instead happens whenever Zola's author decides it's time (I don't know the particular reasoning behind when a release is deemed ready).

The sha256 update commit is now in this PR's branch and I've closed the other pull request. Edit: Only the audit tests are failing now, which is to be expected due to the sha256 change here.

Does the Zola formula need a revision bump or are these changes fine without one? If it's relevant to that question, I feel like the bottles for formulae that don't have the --locked flag should probably be rebuilt when the flag is added, since there's no telling which dependency versions were used in the bottle when it was created.

Besides Zola, there's also another formula with an open issue (pijul) that is stuck on a certain version (since their website hasn't added tar.gz files for newer versions) and needs the --locked flag but I wanted to check with everyone to see if it would be appropriate to move forward with creating a PR for that change or if I should wait to modify it as part of a larger change adding the --locked flag to formulae that have a Cargo.lock file in their source (see my earlier question above about whether the --locked formulae updates should be in one PR, separate PRs, etc.).

Past that, should this discussion be continued in an issue (so that it's still visible after this PR is merged) or should we simply continue on in this PR?

[zbeekman]

Does the Zola formula need a revision bump or are these changes fine without one?

Yes, it does since upstream changed the release assets on us. Not sure what changed within those assets, but a revision bump will ensure that users get the version of Zola that upstream intended to release.

[samford]

I reworked the commit updating the sha256 to also bump the revision. Does this PR need anything else?

[zbeekman]

I reworked the commit updating the sha256 to also bump the revision. Does this PR need anything else?

PR looks good to me! Audits may still fail, we'll see if the rev bump prevents it or not. Since we've confirmed with upstream already that they pulled the src and then republished it, we can safely ignore the audit if it complains about the changed SHA256 checksum.

[zbeekman]

lol. Client side UI hiccup. Didn't mean to spam approve the PR.

[samford]

The sha256-related audits still fail with the revision bump but everything else seems fine.

I've created a new issue (#46025) to continue the discussion around the --locked flag, track progress on updating related formulae, and so that there's something for people to see and easily reference once this PR is merged and becomes less visible.

Thanks to everyone for all their help here.

[zbeekman]

Thanks for your contribution to Homebrew! 🎉 🥇

Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect. Thank you @samford!!!