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

Enable usage of machine set CRATES var #13

Merged

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Aug 21, 2024

Repos using the run_task script would like to set the CRATES env var by using

REPO_DIR=$(git rev-parse --show-toplevel)
CRATES="$(cargo metadata --no-deps --format-version 1 | jq -j -r '.packages | map(.manifest_path | rtrimstr("/Cargo.toml") | ltrimstr("'"$REPO_DIR"'/")) | join(" ")')"

I don't know exactly why but this results in some sort of square braces data type that seems to only work in a loop using for crate in $CRATES instead of the for crate in ${CRATES[@]} like we currently have.

@tcharding tcharding marked this pull request as draft August 21, 2024 06:46
@tcharding tcharding force-pushed the 08-21-ci-generate-crates-list branch from ec4ccd7 to 7fb5d9c Compare August 21, 2024 06:46
@apoelstra
Copy link
Member

Using $PWD like that will mean that the script doesn't work if it's run from the wrong directory. I think we have an env var set somewhere already which uses git rev-parse --show-toplevel or something to orient our scripts around a fixed directory.

@tcharding
Copy link
Member Author

I'll come back to this after #11 merges.

Repos using the `run_task` script would like to set the `CRATES` env var
by using

CRATES="$(cargo metadata --no-deps --format-version 1 | jq -j -r '.packages | map(.manifest_path | rtrimstr("/Cargo.toml") | ltrimstr("'"$PWD"'/")) | join(" ")')"

I don't know exactly why but this results in some sort of square braces
data type that seems to only work in a loop using `for crate in $CRATES`
instead of the `for crate in ${CRATES[@]}` like we currently have.
@tcharding tcharding force-pushed the 08-21-ci-generate-crates-list branch from 7fb5d9c to 534e497 Compare August 29, 2024 01:51
@tcharding tcharding marked this pull request as ready for review August 29, 2024 02:01
@tcharding
Copy link
Member Author

The commit hash from this branch is now used in rust-bitcoin/rust-bitcoin#3201 to verify this works using CI.

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 534e497

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 534e497

@tcharding
Copy link
Member Author

Shall we give @Kixunil a chance to take a look, especially since I copied his code to get the looping to work?

Copy link

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 534e497

However I wonder why a global variable is used and why isn't the command to obtain the list of crates ran by this script.

@@ -64,7 +64,7 @@ main() {
# can't find the file because of the ENV var
# shellcheck source=/dev/null
. "$crates_script"
for crate in "${CRATES[@]}"; do
for crate in $CRATES; do
Copy link

Choose a reason for hiding this comment

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

A different way to do that would be to just load the output of that command into an array. However doing that just so you can have a space in crate directory is silly, so this is fine.

@apoelstra
Copy link
Member

However I wonder why a global variable is used and why isn't the command to obtain the list of crates ran by this script.

It would be nice if it would default to using the command-generated list of crates (and same for a feature matrix) but we'll definitely want a way to override the command for some repos, at least temporarily.

@apoelstra
Copy link
Member

Also Kix ought to be a maintainer on this repo.

@tcharding tcharding merged commit f92b276 into rust-bitcoin:master Sep 1, 2024
1 check passed
@tcharding tcharding deleted the 08-21-ci-generate-crates-list branch September 26, 2024 23:07
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.

3 participants