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

Replace version_check with our own parser to generate better errors #26

Merged
merged 2 commits into from
Aug 31, 2020

Conversation

taiki-e
Copy link
Owner

@taiki-e taiki-e commented Aug 31, 2020

This helps us to understand what kind of error occurred at which stage of parsing. See also #24 (comment).

Comment on lines +36 to +42
// from the verbose version output
fn from_vv(vv: &str) -> Option<Self> {
// Find the release line in the verbose version output.
let release = vv
.lines()
.find(|line| line.starts_with("release: "))
.map(|line| &line["release: ".len()..])?;
Copy link
Owner Author

Choose a reason for hiding this comment

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

This parses the "release: " line of the verbose version output.
This line doesn't contain extra info like the commit date, so it may be a bit more robust than the previous way.

(I suspect that the cause of #24 may be that rustc installed by yum added its own information to the output of rustc --version. So, this may solve #24 completely)

Copy link

@APIPLM APIPLM Aug 31, 2020

Choose a reason for hiding this comment

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

@taiki-e Okay. I will merge those to local project, and test out. I have come back to the situation, I mean install rustc and cargo version 1.45.2 by yum not by rustup.

Copy link

@APIPLM APIPLM Sep 1, 2020

Choose a reason for hiding this comment

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

@taiki-e Yes. after coming back to the incorrect situation, where install rustc and cargo version 1.45.2 by yum NOT rustup. I build the older version (0.4.1)of taiki-e/const_fn, the errors came out again. That means we reduplicate that case. Then download the latest version (0.4.2) of taiki-e/const_fn, and build it and success. Then move on to build time-rs/time with updating taiki-e/const_fn the latest version (0.4.2). It success. As I check the updated source code, it like remove the dependence version_check v0.9.2 by having your code inside version check in the file build.rs. One more thing I noticed that the other crate in time-rs/time v0.2.16 has the dependence version_check v0.9.2 as I saw the downloading version_check v0.9.2 and compiling, but there is no compile issue with the case(installing rustc and cargo version 1.45.2 by yum NOT rustup).
Anyway. Thanks for your help

Copy link
Owner Author

Choose a reason for hiding this comment

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

@APIPLM Thanks for confirming it. Anyway, I believe #24 is completely resolved so I'll close that.

One more thing I noticed that the other crate in time-rs/time v0.2.16 has the dependence version_check v0.9.2 as I saw the downloading version_check v0.9.2 and compiling, but there is no compile issue with the case(installing rustc and cargo version 1.45.2 by yum NOT rustup).

Even if time depends on version_check, it doesn't fail to compile due to the same problem, because the build script is written so that compile will continue even if version_check fails to determine version. AFAIK, this is a common design used by most crates.
The reason const-fn is designed to make this fail is that it is likely a bug in version parser or this crate itself. (rustversion is also designed to stop compilation as well)

@taiki-e
Copy link
Owner Author

taiki-e commented Aug 31, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 31, 2020

Build succeeded:

@bors bors bot merged commit ce4d34c into master Aug 31, 2020
@bors bors bot deleted the version branch August 31, 2020 07:48
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.

2 participants