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

ci: stop using jq, calculate which crates use cargo deb #104

Merged
merged 5 commits into from
May 22, 2024

Conversation

TheButlah
Copy link
Collaborator

@TheButlah TheButlah commented May 21, 2024

In #101 and #103, I agreed to port the code from jq to pure python. This pr implements that change and also merges the two helper python scripts into one file.

I also fixed a regression where not all cargo debs were being produced, and not all binaries were getting copied into the artifacts dir.

@TheButlah TheButlah force-pushed the thebutlah/port-from-jq branch 3 times, most recently from 6ac4bb2 to 5480d81 Compare May 21, 2024 18:27
@TheButlah TheButlah requested a review from andronat May 21, 2024 18:40
@TheButlah TheButlah marked this pull request as draft May 21, 2024 18:50


def run_with_stdout(command, print_cmd=True):
assert isinstance(command, str)
Copy link
Member

Choose a reason for hiding this comment

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

That's kinda a useless check. very broad.

Copy link
Collaborator Author

@TheButlah TheButlah May 22, 2024

Choose a reason for hiding this comment

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

It seems beneficial for error detection. What is the recommended way to do this, if I'd like a nicer error message / more safety than duck typing?

scripts/rust_ci_helper.py Outdated Show resolved Hide resolved
assert exit_code == 0


def run_with_stdout(command, print_cmd=True):
Copy link
Member

Choose a reason for hiding this comment

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

as above, i would personally just keep 1 run command that gets all stdout/err and print stuff. if you want to be "lean" you can only print stuff in case of error.

Copy link
Collaborator Author

@TheButlah TheButlah May 22, 2024

Choose a reason for hiding this comment

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

The reason why run_with_stdout and run both exist is that the latter captures stdout and wouldn't print it until execution is finished. Whereas the former just captures the error code, and therefore stdout is streamed in realtime. I could create a way to unify these two such that stdout is always printed in realtime, while still letting me capture the output as a python string, but that is more work than I think is worth.

Additionally, sometimes we really don't want to print the stdout. If I run the script on my machine and print the output of the metadata command for example, the results are so big that it lags/crashes the terminal multiplexer. We really do just want to pipe it to python and post-process it silently sometimes.

I've pushed a change that always prints the command being run though, and removes the print_cmd flag.

@TheButlah TheButlah marked this pull request as ready for review May 22, 2024 20:58
@TheButlah TheButlah requested a review from andronat May 22, 2024 20:58
@TheButlah TheButlah enabled auto-merge (squash) May 22, 2024 20:59
@TheButlah
Copy link
Collaborator Author

TheButlah commented May 22, 2024

Side note: writing this script really made me regret not just using rust, for these reasons:

  • Having an actual type checker so I didn't have to guess all the time would have been really nice. I spent a lot of time just running code over and over again until I finally got things to work and guessing function signatures. But I could have done that easier if I had a static type system.
  • I could have just used a crate instead of needing to parse cargo metadata json myself.
  • Pythons support for piping bash functions together is really not very ergonomic, I would much rather use cmd_lib.
  • The python script can't easily use packages unless using another package manager. In our case that would be nix, but poetry could have worked too. However we don't currently use poetry, and nix is not ideal especially when adding dependencies on first-party repos. If we were using rust, we can use cargo which has excellent support for git dependencies across repos. This is important for example to reuse this same script in orb-internal - I just add a git dependency on orb-software, instead of having to copy the same python file between repos.
  • Cargo xtask allows us to easily call these sorts of project-specific tools, and bootstrap everything from just raw cargo.

I will not be making the mistake of python helper scripts again, and will port this in the future to just be a cargo xtask.

I might have felt differently if this repo didn't already require cargo and if everyone was using python extensively already and the package managers and venvs and stuff was already set up. Then I'm not sure it would make sense to use rust. But since its primarily a rust codebase, I think sticking to rust where possible makes sense.

Copy link
Member

@andronat andronat left a comment

Choose a reason for hiding this comment

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

@TheButlah TheButlah merged commit 0959f7f into main May 22, 2024
9 checks passed
@TheButlah TheButlah deleted the thebutlah/port-from-jq branch May 22, 2024 22:33
@TheButlah TheButlah changed the title ci: use subcommands in helper and stop using jq ci: use subcommands in helper, stop using jq, calculate which crates use cargo deb May 28, 2024
@TheButlah TheButlah changed the title ci: use subcommands in helper, stop using jq, calculate which crates use cargo deb ci: stop using jq, calculate which crates use cargo deb May 28, 2024
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.

2 participants