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 test for condition_script_runner_args expansion #1157

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wmmc88
Copy link
Contributor

@wmmc88 wmmc88 commented Sep 4, 2024

Added an additional test for functionality that #1132 added. This was made possible by a fix included in rust-script v 0.35.0 that fixes fornwall/rust-script#135

let step = Step {
name: "test".to_string(),
config: Task {
install_crate: Some(InstallCrate::CrateInfo(InstallCrateInfo {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

install_crate here doesnt do what i intended here since its run after the condition script is validated. I should just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sagiegurari is there a better way to ensure that rust-script 0.35.0 is available for this test?

Copy link
Owner

Choose a reason for hiding this comment

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

you can create multiple tasks with dependencies.
so one task will do install and another task depending on the first, will do the condition.

@wmmc88
Copy link
Contributor Author

wmmc88 commented Sep 5, 2024

@sagiegurari It looks like the test is failing only on MSRV rust on ubuntu. I (very hackily) forced some additional debug output and it looks like rust-script fails to be installed w/ msrv rust. I see 2 issues with this:

  1. --locked should be passed to install for the scriptrunner. I suspect some dependency of rust-script no-longer compiles with cargo-make's msrv
  2. validate should be set to true for install for the rust-script runner. Right now it is false, so a failure to install rustscript is not correctly propogated upwards.

Do you agree that these changes make sense to make?

@sagiegurari
Copy link
Owner

@wmmc88 yes both changes (lock and validate) makes sense. do you want to PR that?

@wmmc88
Copy link
Contributor Author

wmmc88 commented Sep 14, 2024

@wmmc88 yes both changes (lock and validate) makes sense. do you want to PR that?

Yes. I'll update this pr with the changes

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 91.83673% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.46%. Comparing base (0e4a95d) to head (d205782).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
src/lib/runner_test.rs 91.66% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
- Coverage   92.82%   92.46%   -0.37%     
==========================================
  Files         119      119              
  Lines       26302    26344      +42     
==========================================
- Hits        24416    24360      -56     
- Misses       1886     1984      +98     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -15,7 +15,7 @@ jobs:
strategy:
fail-fast: false
matrix:
rust: [stable, beta, nightly, 1.73.0]
rust: [stable, beta, nightly, 1.74.0]
Copy link
Contributor Author

@wmmc88 wmmc88 Sep 16, 2024

Choose a reason for hiding this comment

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

rust-script's msrv is now 1.74:

error: failed to compile `rust-script v0.35.0`, intermediate artifacts can be found at `/tmp/cargo-install75pEMP`.
To reuse those artifacts with a future compilation, set the environment variable `CARGO_TARGET_DIR` to that path.

Caused by:
  package `clap_lex v0.7.2` cannot be built because it requires rustc 1.74 or newer, while the currently active rustc version is 1.73.0

@wmmc88 wmmc88 marked this pull request as ready for review September 16, 2024 00:29
@wmmc88
Copy link
Contributor Author

wmmc88 commented Sep 16, 2024

Yes. I'll update this pr with the changes

@sagiegurari The changes are implemented. MSRV was also bumped one version to 1.74 to accomodate for rust-script bumping its MSRV

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.

RUST_SCRIPT_BASE_PATH reports the wrong path when rust-script executes with --base-path arg
2 participants