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

Determine rustc version and add it to the report #81

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

Conversation

TheTechRobo
Copy link

@TheTechRobo TheTechRobo commented Aug 18, 2022

This is a 🙋 feature.

Checklist

  • tests pass
  • tests and/or benchmarks are included - is this necessary/possible for this?
  • documentation is changed or added

Context

Fixes #31

Semver Changes

Major release - it changes the Report struct. I don't think 2.0 is released yet, so that would fit perfectly!

I have very little experience contributing to Rust crates, so if I made any mistakes, let me know!

@bjorn3
Copy link

bjorn3 commented Aug 18, 2022

Cargo also sets the RUSTC env var to the path to rustc. cargo -vV may not match rustc -vV. Especially when using a locally built toolchain using rustup toolchain link as in that case the default cargo (which is probably an official version) will be combined with the local rustc which in most cases has a huge version skew.

@TheTechRobo
Copy link
Author

Cargo also sets the RUSTC env var to the path to rustc.

Did not know that! I'll update that right now.

@TheTechRobo
Copy link
Author

@bjorn3 - getting this issue:

   Compiling human-panic v1.0.4-alpha.0 (/home/thetechrobo/human-panic)
error: environment variable `RUSTC` not defined
 --> /home/thetechrobo/human-panic/build.rs:6:22
  |
6 |     let cargo_path = env!("RUSTC");
  |                      ^^^^^^^^^^^^^
  |
  = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `human-panic` due to previous error

I'm on nightly.

@bjorn3
Copy link

bjorn3 commented Aug 18, 2022

You should use std::env::var("RUSTC").unwrap() instead. RUSTC is passed at runtime of the build script, but not at compile time.

@TheTechRobo TheTechRobo changed the title Determine Cargo version and add it to the report Determine rustc version and add it to the report Aug 18, 2022
@TheTechRobo
Copy link
Author

@bjorn3 - All done.

Copy link

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

LGTM apart from one nit. Do note that I am not a member of this org, so you will have to wait on someone who is to review and merge.

README.md Outdated Show resolved Hide resolved
@TheTechRobo
Copy link
Author

Should I rebase this PR, or are the maintainers fine with 7 (maybe more later if there's more review) commits?

@bjorn3
Copy link

bjorn3 commented Aug 18, 2022

It probably won't hurt to squash. No idea what the maintainers prefer though.

@TheTechRobo
Copy link
Author

I'll probably have time to rebase this on Sunday.

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.

🙋 Including rustc version in report
3 participants