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 cargo workspaces #3602

Merged
merged 6 commits into from
Mar 18, 2024
Merged

Conversation

AyanSinhaMahapatra
Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra commented Nov 21, 2023

Fixes #3598

Tasks

  • Reviewed contribution guidelines
  • PR is descriptively titled 📑 and links the original issue above 🔗
  • Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
    Run tests locally to check for errors.
  • Commits are in uniquely-named feature branch and has no merge conflicts 📁
  • Looked for possible updates in documentation and added updates if applicable
  • Updated CHANGELOG.rst (if applicable)

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
@AyanSinhaMahapatra AyanSinhaMahapatra marked this pull request as draft November 21, 2023 12:02
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
@AyanSinhaMahapatra AyanSinhaMahapatra marked this pull request as ready for review November 21, 2023 19:01
Reference: #3598

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
Copy link
Member

@JonoYang JonoYang left a comment

Choose a reason for hiding this comment

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

@AyanSinhaMahapatra Looks good to me, I left a comment about adding a newline

src/packagedcode/cargo.py Show resolved Hide resolved
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

LGTM.... with a nit wrt. using "workspace" as a placeholder value?
Could we use something better, may be using extra data instead for fields that should be copied back from the workspace in a lower package?

Also would the top level workspace exist as a package of its own or not?

src/packagedcode/cargo.py Outdated Show resolved Hide resolved
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
@AyanSinhaMahapatra
Copy link
Member Author

Could we use something better, may be using extra data instead for fields that should be copied back from the workspace in a lower package?

Yeah this is much better and correct, doing this now.

Also would the top level workspace exist as a package of its own or not?

So I did consider doing this, but the top-level workspace usually does not have any purl fields there, only some extra data wrt source/licenses, see https://github.com/nexB/scancode-toolkit/blob/support-cargo-workspaces/tests/packagedcode/data/cargo/cargo-with-workspace/Cargo.toml#L12 for example. So for now we are not adding top-level packages here, but we can revisit this if we get better examples. But if there are other manifests of other flavours, we'll be adding packages for those.

@AyanSinhaMahapatra AyanSinhaMahapatra added this to the v32.1 milestone Jan 8, 2024
@AyanSinhaMahapatra AyanSinhaMahapatra force-pushed the support-cargo-workspaces branch from 28aeebe to 50d2afe Compare March 18, 2024 03:58
@AyanSinhaMahapatra AyanSinhaMahapatra force-pushed the support-cargo-workspaces branch from 8524736 to d626332 Compare March 18, 2024 04:30
@AyanSinhaMahapatra
Copy link
Member Author

I've removed the improved package assembly part from this PR and pushed this as a separate branch at: https://github.com/nexB/scancode-toolkit/tree/update-package-assembly, since this was not ready to merge. Otherwise this was reviewed and approved, so merging!

@AyanSinhaMahapatra AyanSinhaMahapatra merged commit 0c9fd4b into develop Mar 18, 2024
34 checks passed
@AyanSinhaMahapatra AyanSinhaMahapatra deleted the support-cargo-workspaces branch March 18, 2024 05:23
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.

Do not crash with Cargo workspaces
3 participants