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

fix: preserve workspace inherited dependencies when building binary #1739

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

HerringtonDarkholme
Copy link
Contributor

I don't have enough context for the fix but it did build on my local machine.

Before:
image

After:
image

fix #1738

@netlify
Copy link

netlify bot commented Aug 17, 2023

Deploy Preview for maturin-guide ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c85faa1
🔍 Latest deploy log https://app.netlify.com/sites/maturin-guide/deploys/64dd953c2ceb870008876273
😎 Deploy Preview https://deploy-preview-1739--maturin-guide.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@messense
Copy link
Member

Thanks! I think the issue was

// See https://github.com/toml-rs/toml/issues/594
dep_table.remove(&dep_name);
dep_table[&dep_name] = workspace_dep;

remove(&dep_name) didn't not correctly remove [dependencies] xxx.workspace = true while it works fine for

[dependencies.xxx]
workspace = true
# other fields

@messense messense added the sdist Source distribution label Aug 17, 2023
@HerringtonDarkholme
Copy link
Contributor Author

HerringtonDarkholme commented Aug 17, 2023

If I read the code correct,

// See https://github.com/toml-rs/toml/issues/594
dep_table.remove(&dep_name);
dep_table[&dep_name] = workspace_dep;

this line will only remove workspace deps that are not path deps, since it is inside this check.

// If a workspace inherited dependency isn't a path dep,
// we need to replace `workspace = true` with its full requirement spec.
if !known_path_deps.contains_key(&dep_name) {

However, ast-grep-core is a path dep. So it is not removed.
I wonder what is the desired behavior? Previously all the workspace deps will be removed.

The comment here also specifies that the workspace deps should be removed and have been converted into path deps.

if workspace_inherit {
// Remove workspace inheritance now that we converted it into a path dependency
dep_table[&dep_name]
.as_table_like_mut()
.unwrap()
.remove("workspace");
}

@messense
Copy link
Member

Right, we need to remove all workspace = true because the main Cargo.toml in sdist can't refer to any workspace members because we have rewritten the project structure to put evey path dep in a subdirectory.

It's a bit fragile, I do want to stop doing that, just lift pyproject.toml to sdist top level and rewrite manifest-path instead.

@messense messense merged commit 698e7cb into PyO3:main Aug 17, 2023
31 checks passed
@HerringtonDarkholme
Copy link
Contributor Author

Thanks for the explanation!
By the way it's really a joy to read your code. Thanks for the project 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdist Source distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

maturin >= 1.2.1 fails to build workspace dependencies
2 participants