-
-
Notifications
You must be signed in to change notification settings - Fork 80
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
support workspace version #371
Conversation
Signed-off-by: Alex Chi <iskyzh@gmail.com>
Sorry for the bug and thanks for the fix! I'd be happy to merge and release this ASAP. Any idea why it's failing in CI? It seems to be unable to find the generated rustdoc file for one of our tests. I'm not familiar with workspace versions so I figured I'd ask in case something stood out to you. I'd also love to have the crate and version where this broke to our integration test suite, so this doesn't ever happen again. You're welcome to add it or I can do it right before merging. |
Sure! I'll investigate the CI failure and add our test cases tonight... |
This looks like a parsing bug of upstream library
is parsed as:
where the lib name is wrong. I'll see how to workaround this... |
That's a bummer :( It might be a good idea to open an issue on their issue tracker if you have a chance, since the repo seems decently active: https://gitlab.com/crates.rs/cargo_toml/-/issues |
Signed-off-by: Alex Chi <iskyzh@gmail.com>
Signed-off-by: Alex Chi <iskyzh@gmail.com>
I've workaround this issue by first parsing directly with toml, and then cargo_toml. I'm going to submit a fix to upstream and meanwhile I think this workaround will work for now, unless someone uses both renamed lib and workspace inheritance 🤣 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good so far. Unfortunately, it doesn't seem to be a complete solution to the problem.
I added an integration test (with two checks) against the specific library and version that encountered the issue, and as far as I can tell both checks are failing at the moment.
src/manifest.rs
Outdated
let manifest_text = std::fs::read_to_string(&path) | ||
.map_err(|e| anyhow::format_err!("Failed when reading {}: {}", path.display(), e))?; | ||
let parsed = toml::from_str(manifest_text.as_str()) | ||
.map_err(|e| anyhow::format_err!("Failed to parse {}: {}", path.display(), e))?; | ||
|
||
let parsed = match toml::from_str(&manifest_text) { | ||
Ok(parsed) => parsed, | ||
Err(_) => { | ||
// Parsing failed, possibly because of workspace inheritance. Retry with cargo_toml. | ||
cargo_toml::Manifest::from_path(&path)? | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do two phase loading instead of directly loading what is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo_toml failed to get the correct library name, and therefore this is a workaround to forcefully overwrite the library name parsed by cargo_toml. I have an upstream PR. Once it's fixed we can remove this workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had missed it because the comment is not on the weird case but on the expected case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to move it closer 👍
Signed-off-by: Alex Chi <iskyzh@gmail.com>
Just pushed a commit and I believe this will be the real "dirty" fix 🤣 passing integration test for sqllogictest 0.13 locally and let's see if it works on CI. Thanks for your reviews! |
Signed-off-by: Alex Chi <iskyzh@gmail.com>
I'll submit another PR once the upstream fix has been merged and released to crates.io. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the help! I'm going to merge this and queue up a patch release.
close #370