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

Convert shell scripts to Rust #891

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Convert shell scripts to Rust #891

merged 1 commit into from
Feb 22, 2024

Conversation

djkoloski
Copy link
Member

I primarily dev on Windows, and I'd prefer to not have to use WSL to work on zerocopy.

@joshlf
Copy link
Member

joshlf commented Feb 16, 2024

You'll need to resolve the merge conflicts first before we can see whether CI passes.

@@ -109,8 +109,8 @@ jobs:
#
# ...to:
#
# toolchain: $ {{ ./cargo.sh --version matrix.toolchain }} # hypothetical syntax
ZC_TOOLCHAIN="$(./cargo.sh --version ${{ matrix.toolchain }})"
# toolchain: $ {{ cargo run -p cargos --version matrix.toolchain }} # hypothetical syntax
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with Windows enough to know: Is there a way that we could keep a script that would be compatible with both Windows and POSIX, and would just be a thin wrapper around cargo run -p cargos? The latter is more verbose, which isn't great for the command we run the most frequently during development.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could just publish this as a separate package and cargo install it. Another option would be to leave the cargo.sh file in place and make it a thin wrapper over cargo run -p cargos.

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 the cargo.sh option makes sense. If we can make it so that its invocation is unchanged relative to what it currently is on main, that'd be ideal.

tools/cargos/src/main.rs Outdated Show resolved Hide resolved
tools/cargos/src/main.rs Outdated Show resolved Hide resolved
tools/generate-readme/src/main.rs Outdated Show resolved Hide resolved
@djkoloski
Copy link
Member Author

CI is failing with:

Run cargo run -p cargos -- +msrv check --tests --package zerocopy --target i686-unknown-linux-gnu  --verbose
error: failed to select a version for the requirement `regex = "=1.10.3"`
candidate versions found which didn't match: 1.8.4, 1.8.3, 1.8.2, ...
location searched: crates.io index
required by package `generate-readme v0.0.0 (/home/runner/work/zerocopy/zerocopy/tools/generate-readme)`

Although the regex dependency in generate-readme is specified as regex = "1", the Cargo.lock file is being generated on a later toolchain. regex has an MSRV of 1.65.0 as of regex-1.10.0 and an MSRV of 1.60.0 as of regex-1.8.0.

I don't think we should require the tooling to have the same MSRV as zerocopy and zerocopy-derive, but since it's in the workspace it gets added to package resolution. We could remove them from the workspace, but that would also be annoying and make the tools difficult to invoke. It would be nice if we could ignore these crates on +msrv, but I don't see a clear way to do that. We could just regenerate the Cargo.lock too, but that has the potential future problem if a tool adds a dependency that is never supported on the MSRV toolchain.

Another approach would be to bump the MSRV, but I don't know what the motivation for such a low MSRV is (1.56.0 is about two years and four months old). Up to you @joshlf how you want me to approach resolving it.

@@ -109,8 +109,8 @@ jobs:
#
# ...to:
#
# toolchain: $ {{ ./cargo.sh --version matrix.toolchain }} # hypothetical syntax
ZC_TOOLCHAIN="$(./cargo.sh --version ${{ matrix.toolchain }})"
# toolchain: $ {{ ./cargos.sh --version matrix.toolchain }} # hypothetical syntax
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to leave this named cargo.sh? (I'll be honest I'm not sure what the etymology of cargos is as opposed to cargo)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is an unfortunately mundane one. I originally kept it named cargo.sh and made cargo.bat to match. As it turns out though, when cargos runs a cargo command, Windows will run cargo.bat and not the installed cargo binary. So it effectively causes cargos to recursively call itself when it intends to invoke cargo. The easy fix was to use a different name, and I figured if I had to call it cargos on Windows then I should probably just make it uniform.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense. My preference would be to keep cargo.sh named as-is, and rename the .bat file. Maybe win-cargo.bat? I'll leave it up to you what you want to name it.

@joshlf
Copy link
Member

joshlf commented Feb 20, 2024

It looks like the invocation of cargo itself (e.g. the one inside cargos.sh) is using the default toolchain installed in the GitHub Action. You probably want to do cargo +stable like we do in cargo.sh on main.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

I pushed a new commit that should hopefully fix (some of) the CI errors. Feel free to delete it, squash it, etc.

Since these are in their own workspace now, can you update ci/check_fmt.sh to format that workspace as well?

tools/cargos/src/main.rs Outdated Show resolved Hide resolved
tools/cargos/src/main.rs Outdated Show resolved Hide resolved
@joshlf joshlf force-pushed the convert_shell_scripts branch 2 times, most recently from ac19afc to 64b2c16 Compare February 21, 2024 05:57
@djkoloski
Copy link
Member Author

ci/check_fmt.sh picks up the .rs files in the tools/ workspace, so no changes required there.

@djkoloski djkoloski force-pushed the convert_shell_scripts branch 3 times, most recently from 442f6ad to bfb89ea Compare February 22, 2024 15:38
Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Last comments that I'm aware of - once these addressed, we should be ready to merge.

Can you also squash all of these commits together? Otherwise, the entire set of commits shows up in the commit message that GitHub generates.

cargo.sh Outdated Show resolved Hide resolved
`cargo.sh` is now a thin wrapper around a new `cargo-zerocopy` tool
located in the `tools` directory. README generation is now done by a
similar `generate-readme` tool. A wrapper script for running
`cargo-zerocopy` on Windows has been added as `win-cargo.bat`,
effectively allowing the test suites to be run on Windows.
@joshlf joshlf disabled auto-merge February 22, 2024 17:52
@joshlf joshlf added this pull request to the merge queue Feb 22, 2024
Merged via the queue into main with commit 4b7f1df Feb 22, 2024
210 checks passed
@joshlf joshlf deleted the convert_shell_scripts branch February 22, 2024 18: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.

None yet

2 participants