Add xtask msrv #3189
Add xtask msrv #3189
Conversation
There was a problem hiding this comment.
This looks good so far! Let's continue working on this branch.
I think the next step will be to take the new MSRV given on the command-line and substitute it into the necessary files. I think the trick will be to identify the precise spot in those files to write the MSRV, without accidentally changing anything else. It's fine to add some comments to the files that the cargo xtask msrv command can use to find it. For example, in the .github/workflow files, that could be
# MSRV - update with `cargo xtask msrv`
- "1.64"It's probably easiest to do this by reading each file into memory with std::fs::read_to_string, then manipulating it with regular expressions (using the regex crate). But, you could also do this by iterating over lines in the file and using some basic string operations like s.ends_with("# MSRV").
Give that a try, and let me know if you get stuck!
|
Hi @djmitche - happy Friday! Ok, so based on your feedback and some of my own assumptions and updated the code accordingly:
I see that the CICD test Q: Is there a way to run all the CICD tests locally before push my pull request? |
Other than on a platform you have locally, I don't think so. It may be possible to run a codespace in other platforms (like Windows), but I don't have any experience doing so. Still, if tests run locally then it's usually fine to use the PR for testing other platforms. |
There was a problem hiding this comment.
Looking good so far! Hopefully these comments help with next steps.
Worth considering, too: /// is not a comment in YAML, but # is. Can you abstract that away? Or maybe have different functions for .rs and .yml files? We'll also need to modify a .toml file (Cargo.toml) when the rust version is added there.
taskchampion/xtask/src/main.rs
Outdated
| use std::path::{Path, PathBuf}; | ||
|
|
||
| // Increment length of array when adding more paths. | ||
| const MSRV_FILE_PATHS: [&str; 2] = [r"main.rs", r"src/main.rs"]; |
There was a problem hiding this comment.
I can't quite tell where these paths are relative to?
Cargo sets $CARGO_MANIFEST_DIR to the taskchampion/xtask directory, so you might be able to build paths relative to there, and then use that environment variable to make xtask msrv independent of the current directory.
There was a problem hiding this comment.
Oh, I'm sorry. These are left over test paths. One fails, one succeeds.
|
Thank you for taking the time to provide thorough and articulate comments! Will get started on these changes this upcoming week. |
|
Next push to this pr will include three updates:
|
|
Hi @djmitche - I think this is ready for another review. Thanks for your patience.
There are two limitations I am aware of that I didn't address:
Please let me know what else can I clean up before it is ready to be merged and used! |
There was a problem hiding this comment.
There are a few clippy checks below that I've highlighted. But, I'm a bit confused about what this is doing right now. It looks like, in a Rust file for example, it would look for a line like
let x = y; // MSRV = 1.59and if found turn that into
// MSRV = 1.60
let x = y; // MSRV = 1.59Maybe I can give a clearer description of how this typically works in other projects. Somewhere in the middle of ``, there appears
- uses: actions-rs/toolchain@v1
with:
toolchain: "1.64" # MSRV - update with 'cargo xtask msrv'
override: true(I've added the comment; you can do the same in the PR)
So, to update that MSRV:
- look for a line ending with
# MSRV update with 'cargo xtask msrv' - if not found, output the existing line
- if found, replace
[0-9]\.[0-9]{2,}with the new version within the line and output it.
Does that make more sense?
This might be easier to test if you factor out the "replace the MSRV" operation into a function that operates on a string. Then the msrv function can read the file content to a string, call the replacement function, and write the result back out. And tests can just provide a test string to the replacement function.
|
Thanks for the feedback @djmitche . I had incorrectly understood and assumed that the intent was to update comments in files that specify the MSRV to be used, therefore my approach was to look for comments in files that contained Instead, it sounds like the intent is to update any given configuration file's specifications. So, similar to what you wrote above, in - uses: actions-rs/toolchain@v1
with:
# Same as the MSRV in rust-tests.yml
toolchain: "1.64"...the version in the line If that is the case, then my approach would be to look for a different X.Y and update it with the new X.Y where Do I have that correct? |
|
Pushed a commit to fix clippy issues, but still working on functionality to update file contents (instead of just file comments). (Had much less time this week). |
|
I think you have that correct! Here is an example of doing something similar, but in JS. This code writes out a new version number in a bunch of places. The Anyway, within Taskwarrior, as long as we can be pretty confident that only the correct value will be changed, and other parts of the file won't be accidentally modified, any kind of replacement strategy is fine. |
|
Good afternoon @djmitche - I changed out the script action to update the actual version number content of a file instead of managing the comments. I examined the example you linked and found that the onus for configuration is put on the developer to 1) add their file to the path list and 2) specify the regex that is used to properly find the line containing the MSRV. Taking a similar approach in this script, a developer would add the appropriate file path and regex to the CONST tuple array. For which they would need to be responsible for assuring that their provided regex does not result in inadvertent matches within the file they specify.
If there are any further updates needed, I should have more availability next week to get a quicker turn, so thanks for the patience this week as I had less capacity. |
Co-authored-by: Dustin J. Mitchell <dustin@v.igoro.us>
|
Hi @djmitche - I think I've addressed your concerns in your feedback. I assume that a developer would use this manually, infrequently, and specifically within the context of a particular project, so they would only add paths to xtask's So the Because I'm not 100% familiar with how this type of tool gets used and I'm trying not to bother with too many questions, the assumption above may be incorrect. (But I am very grateful for the guidance and patience you've provided!). Please let me know if there is anything else I can address for you! |
This tool would only be used by this repository, so it can definitely be specific to taskwarrior. I'm not sure if that's what you meant by "project" here -- but anything in the repo is fair game.
Yep, that's the directory indicated by CARGO_MANIFEST_DIR.
To be clear, this isn't a general MSRV-management tool. It's meant specifically for this repository, which is why it's OK to hard-code the filenames in the source flie. |
taskchampion/xtask/src/main.rs
Outdated
| const MSRV_PATH_REGEX: [(&str, &str); 1] = [( | ||
| "xtask/Cargo.toml", | ||
| r#"rust-version = "[0-9]+("|\.|[0-9])+""#, | ||
| )]; |
There was a problem hiding this comment.
Using a slice here, instead of an array, removes the need to specify the length. Let's also use the actual files we'd like to modify.
| const MSRV_PATH_REGEX: [(&str, &str); 1] = [( | |
| "xtask/Cargo.toml", | |
| r#"rust-version = "[0-9]+("|\.|[0-9])+""#, | |
| )]; | |
| const MSRV_PATH_REGEX: &[(&str, &str)] = &[ | |
| ( | |
| "../.github/workflows/checks.yml", | |
| r#"toolchain: "[0-9.]+*" # MSRV"#, | |
| ), | |
| ("../.github/workflows/rust-tests.yml", r#""[0-9.]+" # MSRV"#), | |
| ( | |
| "taskchampion/src/lib.rs", | |
| r#"Rust version [0-9.]* and higher"#, | |
| ), | |
| ]; |
I also made the following changes in the github workflow files, so that the regexes above will work:
diff --git .github/workflows/checks.yml .github/workflows/checks.yml
index b1370c905..64de42ce6 100644
--- .github/workflows/checks.yml
+++ .github/workflows/checks.yml
@@ -24,19 +24,17 @@ jobs:
- name: Cache cargo build
uses: actions/cache@v3
with:
path: target
key: ${{ runner.os }}-cargo-build-target-${{ hashFiles('**/Cargo.lock') }}
- uses: actions-rs/toolchain@v1
with:
- # Fixed version for clippy lints. Bump this as necesary. It must not
- # be older than the MSRV in tests.yml.
- toolchain: "1.64"
+ toolchain: "1.64" # MSRV
override: true
- uses: actions-rs/cargo@v1.0.3
with:
command: check
- run: rustup component add clippy
diff --git .github/workflows/rust-tests.yml .github/workflows/rust-tests.yml
index 776642ee6..be2b7c711 100644
--- .github/workflows/rust-tests.yml
+++ .github/workflows/rust-tests.yml
@@ -10,18 +10,17 @@ on:
pull_request:
types: [opened, reopened, synchronize]
jobs:
test:
strategy:
matrix:
rust:
- # MSRV; must not be higher than the clippy rust version in checks.yml
- - "1.64"
+ - "1.64" # MSRV
- "stable"
os:
- ubuntu-latest
- macOS-latest
- windows-latest
name: "rust ${{ matrix.rust }} on ${{ matrix.os }}"
runs-on: ${{ matrix.os }}There was a problem hiding this comment.
I'm assuming I don't need to include the changes to these configuration files as part of my own PR, but please let me know if that is incorrect. I did change the array to a slice per your recommendation, which makes sense and hadn't considered that as an option.
There was a problem hiding this comment.
Yes, please do include these changes in .github as well -- without them, cargo xtask msrv won't update these files, so arguably the job isn't finished :)
|
This is looking great! I think the Thanks for sticking with this through all the revisions.. |
|
Hi @djmitche I updated the two .yml files . I also noticed in I also see that |
|
Did you have ideas what you'd like to work on next? |
|
I haven't quite figured it out yet. Here are some unvetted / initial ideas: Unvetted Project Ideas
ObjectivesThe ideal project is an intersection of the following:
Primary Goal:
Secondary Goals:
Advice?I've been embarking on a journey to build, hone, and develop enough coding skill to call myself and actual 'programmer' (context). If I were an intern on your team in a professional setting, what feedback / advice would you give me? If you don't mind, I'll ping you on Discord for a personal, non-public, response. FYII'll be writing a short blog post about what I learned building xtask MSRV. |
Description
Adds a placeholder function for xtask called msrv with arguments (x,y) that starts work on taskwarrior issue 3142: Automate synchronization of MSRV
Additional information...
I added placeholder function with 'hello world' and x,y parameter output.
I ran
cargo run msrv arg1 arg2to confirm function is called and outputs as expectedI ran cargo test to confirm no warnings or errors