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.

Sign up for GitHub

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

Jump to bottom

cargo: Handle excluded paths in workspaces #323

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cargo: Handle excluded paths in workspaces #323

wants to merge 1 commit into from

Conversation

gasinvein
Copy link
Member

@gasinvein gasinvein commented Oct 9, 2022 •
edited
Loading

Should fix #322

@gasinvein gasinvein mentioned this pull request Oct 9, 2022
Closed
@gasinvein gasinvein marked this pull request as ready for review October 10, 2022 10:43
@gasinvein gasinvein force-pushed the cargo-excluded-packages branch 2 times, most recently from 23797ed to 19bba11 Compare October 15, 2022 09:58
@hfiguiere
Copy link
Collaborator

@gasinvein is this still valid since the fixed bug has been closed?

@gasinvein
Copy link
Member Author

Yes, the issue was closed by mistake I believe.

@hfiguiere hfiguiere mentioned this pull request Apr 5, 2024
Merged
@hfiguiere hfiguiere added the cargo Rust cargo label Apr 5, 2024
zecakeh
zecakeh reviewed Apr 6, 2024
View reviewed changes
cargo/flatpak-cargo-generator.py
@@ -146,6 +153,9 @@ async def get_dep_packages(entry, toml_dir):
if dep_name in packages:
continue
dep_dir = os.path.normpath(os.path.join(toml_dir, dep['path']))
if is_excluded(dep_dir):
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that an excluded member of a workspace is just excluded from the list of members of the workspace. If a package is listed as a dependency, it will be needed during compilation so it must be resolved somehow so it doesn't make sense to not try to load it.

@zecakeh
Copy link
Contributor

zecakeh commented Apr 6, 2024 •
edited
Loading

I believe that the main problem in #322 is that the script eagerly loads the manifest of every package in a git source and tries to resolve all of its dependencies, even if we don't need the package in the end.

When we encounter a package, we should always check if it's listed in the Cargo.lock, otherwise we can safely skip it. So I see two options:

  1. Keep the list of packages in Cargo.lock around and check it every time, it would probably be the simplest as it only involves passing a list around
  2. Load the source first, and then resolve only the packages we need lazily, but that might probably involve deeper changes.

Edit: Actually, it seems like #399 would fix #322.

@hfiguiere
Copy link
Collaborator

#399 was merged. And other fixes too. Maybe it's worth retrying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zecakeh zecakeh zecakeh left review comments

Assignees
No one assigned
Labels
cargo Rust cargo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

flatpak-cargo-builder fails because Cargo.toml in git submodule is not found
3 participants
@gasinvein @hfiguiere @zecakeh