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

Provide more detailed error message when env-script fails #463

Merged
merged 1 commit into from
Oct 3, 2020

Conversation

epontan
Copy link
Contributor

@epontan epontan commented Oct 1, 2020

There is limited information given in the error output when an
env-script return a non-zero exit code, which can be quite confusing as
there is no context of where it failed (unless verbose output is turned
on).

Avoid confusion by providing more context about the environment when the
script fails.

BEFORE:

cargo make
[cargo-make] INFO - cargo make 0.32.5
[cargo-make] ERROR - Error while executing command, exit code: 128
[cargo-make] WARN - Build Failed.

AFTER:

cargo make
[cargo-make] INFO - cargo make 0.32.5
[cargo-make] ERROR - Error while evaluating script for env: GIT_ROOT, exit code: 128
script: [
"git rev-parse --show-toplevel",
]
stdout: ""
stderr: "fatal: not a git repository (or any of the parent directories): .git
"
[cargo-make] WARN - Build Failed.

@sagiegurari
Copy link
Owner

That's really great. Let me check the code out.

Copy link
Owner

@sagiegurari sagiegurari left a comment

Choose a reason for hiding this comment

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

looks great. can we play with the output formatting a bit?

Comment on lines 46 to 47
"stdout: \"{}\"\n",
"stderr: \"{}\""
Copy link
Owner

Choose a reason for hiding this comment

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

can we drop the wrapper " here? its a bit confusing in the output.
we can do
stdout:\n{}\n
stderr:\n{}\n
instead. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I couldn't really decide one ideal output format anyway.

error!(
concat!(
"Error while evaluating script for env: {}, exit code: {}\n",
"script: {:#?}\n",
Copy link
Owner

Choose a reason for hiding this comment

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

can we change this one to
script:\n{:#?}\n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, np.

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2020

Codecov Report

Merging #463 into 0.32.6 will increase coverage by 27.70%.
The diff coverage is 90.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           0.32.6     #463       +/-   ##
===========================================
+ Coverage   64.91%   92.61%   +27.70%     
===========================================
  Files          92       92               
  Lines       18049    18057        +8     
===========================================
+ Hits        11717    16724     +5007     
+ Misses       6332     1333     -4999     
Impacted Files Coverage Δ
src/lib/environment/mod_test.rs 96.15% <83.33%> (+83.90%) ⬆️
src/lib/environment/mod.rs 88.95% <100.00%> (+60.41%) ⬆️
src/lib/logger.rs 85.71% <0.00%> (+2.19%) ⬆️
src/lib/types.rs 85.59% <0.00%> (+2.61%) ⬆️
src/lib/scriptengine/mod.rs 94.16% <0.00%> (+2.91%) ⬆️
src/lib/cli_commands/list_steps.rs 97.05% <0.00%> (+2.94%) ⬆️
src/lib/environment/crateinfo_test.rs 100.00% <0.00%> (+4.30%) ⬆️
src/lib/installer/crate_version_check.rs 74.74% <0.00%> (+5.05%) ⬆️
src/lib/logger_test.rs 97.24% <0.00%> (+5.50%) ⬆️
src/lib/installer/cargo_plugin_installer.rs 82.55% <0.00%> (+6.97%) ⬆️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09473f9...76364b2. Read the comment docs.

There is limited information given in the error output when an
env-script return a non-zero exit code, which can be quite confusing as
there is no context of where it failed (unless verbose output is turned
on).

Avoid confusion by providing more context about the environment when the
script fails.

BEFORE:
 > cargo make
 [cargo-make] INFO - cargo make 0.32.5
 [cargo-make] ERROR - Error while executing command, exit code: 128
 [cargo-make] WARN - Build Failed.

AFTER:
 > cargo make
 [cargo-make] INFO - cargo make 0.32.5
 [cargo-make] ERROR - Error while evaluating script for env: GIT_ROOT, exit code: 128
 script:
 [
     "git rev-parse --show-toplevel",
 ]
 stdout:

 stderr:
 fatal: not a git repository (or any of the parent directories): .git

 [cargo-make] WARN - Build Failed.
@sagiegurari
Copy link
Owner

Looks great @epontan
thanks a lot for the PR

@sagiegurari sagiegurari merged commit d3acd50 into sagiegurari:0.32.6 Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants