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 option to continue if an Error occurs in one section #128

Merged
merged 9 commits into from
Aug 8, 2022

Conversation

puzzlewolf
Copy link
Contributor

@puzzlewolf puzzlewolf commented Aug 1, 2022

Closes #124

What has been done:

  • Add a configuration option skip_if_error to the build, git, rustc and sysinfo features, to allow skipping a feature and continuing if an Error occurs. configure_cargo is not fallible, so the option wouldn't make sense there.
  • Emit a cargo:warning if an Error was skipped in this way.
  • Add a test for skip_if_error for the git feature. This adds a new dependency tempfile, which is just needed for this one test of the git feature. Sadly, there is no way to define optional dev-dependencies (Allow optional dev-dependencies rust-lang/cargo#1596), so I deactivated the unused_crate_dependencies lint.
  • Add documentation for the option and mention option_env!.
  • Fix two unrelated nits.

Open questions:

  • I'm not sure if skip_if_error is a good name, please suggest a better one :)
  • It would be nice to add tests for the other features, too. Do you have an idea how to make them Error?
  • Is disabling unused_crate_dependencies okay for you?
  • If you prefer, I'll open a separate PR for the unrelated nits.
  • Please tell me if you'd like the commits squashed :)


let git_config = instructions.git();

let add_entries = || {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite understand why this add_entries doesn't need to be mut like all the others. For example, if I remove the mut from add_entries in configure_build, the compiler says

cargo check --features build
    Checking vergen v7.3.2 (/home/nora/Projects/vergen)                                        i
error[E0596]: cannot borrow `add_entries` as mutable, as it is not declared as mutable
   --> src/feature/build.rs:150:26
    |
124 |     let add_entries = || {
    |         ----------- help: consider changing this to be mutable: `mut add_entries`
...
128 |                     add_config_entries(config, *build_config, &OffsetDateTime::now_utc())?;
    |                                        ------ calling `add_entries` requires mutable binding due to mutable borrow of `config`
...
150 |             let result = add_entries();
    |                          ^^^^^^^^^^^ cannot borrow as mutable
(... analogous reports snipped ...)

but config is borrowed here in the same way as there 🤔? Any advice appreciated 😄

Anyway, this compiles and seems to do the right thing 🤷

@puzzlewolf
Copy link
Contributor Author

Hey, could you please approve the CI run? I'm a first-time contributor to this project, so GitHub prevents me from running CI (and rightly so) :)

@CraZySacX
Copy link
Member

Sorry was on vacation last week. It's running now.

  - This test needs a directory that is not a git repository or
    a subdirectory of a git repository.
  - Use a temporary directory, provided by the tempfile crate.
  - The tempfile dependency is only needed for this one test. It is only
    compiled in the test configuration and if the git feature is active.
    Sadly, there is no way to define optional dev-dependencies[0], so
    deactivate the unused_crate_dependencies lint to allow compilation
    of the other features.

  [0]: rust-lang/cargo#1596
If the "git" feature is not active, no git2 Errors will be generated.
@puzzlewolf
Copy link
Contributor Author

Thank you :) No problem, I hope the vacation was nice 🍹

I messed up formatting, sorry 😓

@CraZySacX
Copy link
Member

That's why the check is there :). I miss formatting all the time.

@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #128 (7dc4d81) into master (160336f) will decrease coverage by 0.70%.
The diff coverage is 74.82%.

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
- Coverage   94.57%   93.86%   -0.71%     
==========================================
  Files          11       11              
  Lines        1105     1189      +84     
==========================================
+ Hits         1045     1116      +71     
- Misses         60       73      +13     
Impacted Files Coverage Δ
src/lib.rs 63.15% <ø> (+5.26%) ⬆️
src/feature/build.rs 92.85% <54.54%> (-7.15%) ⬇️
src/feature/rustc.rs 93.68% <54.54%> (-5.07%) ⬇️
src/feature/si.rs 97.48% <54.54%> (-2.52%) ⬇️
src/feature/git.rs 91.72% <75.90%> (+3.49%) ⬆️
src/config.rs 89.60% <100.00%> (-2.65%) ⬇️
src/gen.rs 99.62% <100.00%> (+0.02%) ⬆️
src/error.rs 69.56% <0.00%> (-4.86%) ⬇️
... and 3 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@CraZySacX CraZySacX merged commit a80a880 into rustyhorde:master Aug 8, 2022
@puzzlewolf
Copy link
Contributor Author

Thanks for merging :) 🎉

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.

Handle VERGEN_GIT_* more gracefully if the source is not a git repo
2 participants