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

Add a script for updating lock files #2012

Merged
merged 1 commit into from Aug 24, 2023

Conversation

tcharding
Copy link
Member

Every time one of the dependencies is explicitly changed we have to update the minimal/recent lock files. Add a script to do so.

@apoelstra
Copy link
Member

Can you

  • use /usr/bin/env bash rather than /bin/bash
  • add set -e
  • move the existing Cargo.lock file out of the way (as a bonus you can try to restore it even on error; the chatbot should know how to do this in bash)

@dpc
Copy link
Contributor

dpc commented Aug 21, 2023

use /usr/bin/env bash rather than /bin/bash

All NixOS users say: thank you.

set -euo pipefail is a gold standard, good habit, afaik.

BTW. If you drop these scripts in a justfile like this one, then it easier to discover them, document them, and use them. And it's optional.

@tcharding
Copy link
Member Author

Sweet, thanks lads. Will re-spin

@tcharding
Copy link
Member Author

Oh just is sweet, it can wrap up of all the --all-targets, --workspace drama we had too! Thanks @dpc, TIL.

@tcharding
Copy link
Member Author

I've used the shebang line for you nix blokes and used set -eou pipefail, leaving the "convert to just" for a separate PR.

@apoelstra
Copy link
Member

apoelstra commented Aug 22, 2023

The shebang line is /bin/bin/env :) it needs to be /usr/bin/env.

@tcharding
Copy link
Member Author

Jeez, it is hard to get good help.

@tcharding
Copy link
Member Author

Rebased on master (since there were no acks) and used the correct shebang.


for file in Cargo-minimal.lock Cargo-recent.lock; do
cp --force "$file" Cargo.lock
cargo check --color=always --workspace --all-features --all-targets
Copy link
Member

Choose a reason for hiding this comment

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

I think we need cargo check --tests

Copy link
Member Author

@tcharding tcharding Aug 23, 2023

Choose a reason for hiding this comment

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

I think we get that with --all-targets (includes examples, benches, and tests).

Copy link
Member

Choose a reason for hiding this comment

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

If the goal is just to update the lockfile do we actually need any of these flags?

I'm fine to keep them but in practice I always ctrl-C the cargo check invocation as soon as I see any output, because at that point it's already updated the lockfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha, we definitely don't need --color-always, I also ctrl-C soon as it starts scrolling past. (I just blindly cut n paste my shell alias when I wrote this.)

@yancyribbens
Copy link
Contributor

It feels like there should be some kind of documentation update or at least expand the comment in this file to explain what it means to "update dependencies is explicitly " and when to use this script.

@apoelstra
Copy link
Member

You would use this script when you'd otherwise run these commands manually.

apoelstra
apoelstra previously approved these changes Aug 23, 2023
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK e31aa44

@apoelstra
Copy link
Member

@yancyribbens basically you would run this script if you ever edit Cargo.toml. It would probably be good to add a comment to Cargo.toml about this actually..

@tcharding
Copy link
Member Author

Removed --collor=alwas and also the unnecessary explicit exit 0.

@apoelstra
Copy link
Member

FWIW I'm pretty sure none of --workspace --all-features --all-targets are needed either, since the lockfile gets updated before Cargo decides anything else.

apoelstra
apoelstra previously approved these changes Aug 24, 2023
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 1297089

@tcharding
Copy link
Member Author

FWIW I'm pretty sure none of --workspace --all-features --all-targets are needed either, since the lockfile gets updated before Cargo decides anything else.

So obvious once I read it :( Plain old cargo check will run faster too, fixing ...

Every time one of the dependencies is explicitly changed we have to
update the minimal/recent lock files. Add a script to do so.
@apoelstra
Copy link
Member

Lol, I'm not sure I'd say it's "obvious" -- I mostly know this from reading the cargo source code around lockfiles.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 8b84227

@tcharding
Copy link
Member Author

Oh I thought it was obvious because we just discussed Ctl-c'ing the cargo check command "knowing" the lock file will have been updated already, so flags on exactly which blocks of code to check shouldn't matter.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 8b84227

@yancyribbens
Copy link
Contributor

lgtm

@apoelstra apoelstra merged commit da1836e into rust-bitcoin:master Aug 24, 2023
29 checks passed
@tcharding tcharding deleted the 08-21-add-lockfile-script branch August 24, 2023 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants