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

support workspace version #371

Merged
merged 9 commits into from
Feb 17, 2023
Merged

Conversation

skyzh
Copy link
Contributor

@skyzh skyzh commented Feb 16, 2023

close #370

Signed-off-by: Alex Chi <iskyzh@gmail.com>
@obi1kenobi
Copy link
Owner

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.

@skyzh
Copy link
Contributor Author

skyzh commented Feb 16, 2023

Sure! I'll investigate the CI failure and add our test cases tonight...

@skyzh
Copy link
Contributor Author

skyzh commented Feb 17, 2023

This looks like a parsing bug of upstream library toml_cargo. The Cargo.toml:

[package]
name = "renamed_lib"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[lib]
name = "librenamed"

[dependencies]

is parsed as:

    lib: Some(
        Product {
            path: Some(
                "src/lib.rs",
            ),
            name: Some(
                "renamed_lib",
            ),

where the lib name is wrong. I'll see how to workaround this...

@obi1kenobi
Copy link
Owner

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>
@skyzh
Copy link
Contributor Author

skyzh commented Feb 17, 2023

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 🤣

@skyzh
Copy link
Contributor Author

skyzh commented Feb 17, 2023

Copy link
Owner

@obi1kenobi obi1kenobi left a 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/rustdoc_cmd.rs Outdated Show resolved Hide resolved
src/manifest.rs Outdated
Comment on lines 21 to 29
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)?
}
};
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Owner

Choose a reason for hiding this comment

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

@epage is there something I could add or change about the comment above this block to make this clearer? I updated it here but it sounds like it was still confusing.

Copy link
Collaborator

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.

Copy link
Owner

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>
@skyzh
Copy link
Contributor Author

skyzh commented Feb 17, 2023

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>
@skyzh
Copy link
Contributor Author

skyzh commented Feb 17, 2023

I'll submit another PR once the upstream fix has been merged and released to crates.io.

src/manifest.rs Outdated Show resolved Hide resolved
Copy link
Owner

@obi1kenobi obi1kenobi left a 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.

@obi1kenobi obi1kenobi enabled auto-merge (squash) February 17, 2023 21:59
@obi1kenobi obi1kenobi merged commit 5913804 into obi1kenobi:main Feb 17, 2023
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.

workspace version is not supported (regression between 0.17 and 0.18)
3 participants