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

cargo: Allow filtering packages by name #325

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

Conversation

aleb
Copy link

@aleb aleb commented Oct 12, 2022

Cargo.lock might contain unwanted packages.

@gasinvein
Copy link
Member

Can you please explain when and why it could be useful?
Cargo.lock contains complete list of the project's dependencies, and omitting any of them is expected to result in a broken build. So, why someone would want that is definitely not obvious.

@aleb
Copy link
Author

aleb commented Oct 12, 2022

For example out of https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs we want only package "gst-plugin-gtk4", nothing else.

gst-plugins-rs $ ../flatpak-builder-tools/cargo/flatpak-cargo-generator.py Cargo.lock -o x.json
gst-plugins-rs $ ../flatpak-builder-tools/cargo/flatpak-cargo-generator.py Cargo.lock -p gst-plugin-gtk4 -o z.json
gst-plugins-rs $ ls -l *.json
-rw-r--r-- 1 aleb users 248779 12. Okt 21:43 x.json
-rw-r--r-- 1 aleb users  62082 12. Okt 22:57 z.json

@gasinvein
Copy link
Member

I don't see a lockfile in the project you've linked, where does the Cargo.lock come from?

@aleb
Copy link
Author

aleb commented Oct 12, 2022

https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html

If you’re building a non-end product, such as a rust library that other rust packages will depend on, put Cargo.lock in your .gitignore.

@aleb
Copy link
Author

aleb commented Oct 12, 2022

You can cargo fetch to create the Cargo.lock file when it's missing.

@gasinvein
Copy link
Member

gasinvein commented Oct 12, 2022

I still fail to understand what you're trying to achieve and what could be the reason for excluding cargo dependencies from a lockfile.

Just to be clear, to avoid possible misunderstandings: the flatpak-cargo-generator.py script doesn't build anything, neither cause anything to be installed. It only pre-fetches cargo packages cache (via flatpak-builder), very similar to what cargo vendor does (and largely replicating its behaviour). What to build and what not to is still up to Cargo.

@aleb
Copy link
Author

aleb commented Oct 12, 2022

For example out of https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs we want only package "gst-plugin-gtk4", nothing else.

gst-plugins-rs $ cargo build --package 2>&1 | grep gst | sort
    gst-plugin-audiofx
    gst-plugin-aws
    gst-plugin-cdg
    gst-plugin-claxon
    gst-plugin-closedcaption
    gst-plugin-csound
    gst-plugin-dav1d
    gst-plugin-fallbackswitch
    gst-plugin-ffv1
    gst-plugin-file
    gst-plugin-flavors
    gst-plugin-fmp4
    gst-plugin-gif
    gst-plugin-gtk4
    gst-plugin-hlssink3
    gst-plugin-hsv
    gst-plugin-json
    gst-plugin-lewton
    gst-plugin-onvif
    gst-plugin-raptorq
    gst-plugin-rav1e
    gst-plugin-regex
    gst-plugin-reqwest
    gst-plugin-rspng
    gst-plugin-rtpav1
    gst-plugin-sodium
    gst-plugin-spotify
    gst-plugin-textahead
    gst-plugin-textwrap
    gst-plugin-threadshare
    gst-plugin-togglerecord
    gst-plugin-tracers
    gst-plugin-tutorial
    gst-plugin-uriplaylistbin
    gst-plugin-version-helper
    gst-plugin-videofx
    gst-plugin-webp
    gst-plugin-webrtchttp

@gasinvein
Copy link
Member

From a workspace project like this, you select package(s) to build via cargo. Again, flatpak-cargo-generator.py doesn't control what is being built in any way. You can generate sources for the whole gst-plugins-rs project, build still build only desired components.

@aleb
Copy link
Author

aleb commented Oct 12, 2022

Without the filtering we'd have to download a lot of stuff that's not needed, which is wasteful.

@gasinvein
Copy link
Member

cargo vendor doesn't consider "redundant" packages and downloads all the project dependencies, why should we do otherwise?

@aleb
Copy link
Author

aleb commented Oct 14, 2022

Because we can! :)

cargo-vendor "Vendor all dependencies for a project locally". Why does it not allow to vendor only the deps of a single package? That I don't know.

As I said, gst-plugins-rs contains multiple libraries but we only need [to build] only one of them, not all. That's why we download only the dependencies for the corresponding package, not for all the packages in the project.

Comment on lines +297 to +298
for dep in packages_dependencies(deps, all_packages):
yield dep
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for dep in packages_dependencies(deps, all_packages):
yield dep
yield from packages_dependencies(deps, all_packages)

@@ -305,12 +316,22 @@ async def generate_sources(cargo_lock, git_tarballs=False):
VENDORED_SOURCES: {'directory': f'{CARGO_CRATES}'},
}

pkg_coros = [get_package_sources(p, cargo_lock, git_repos) for p in cargo_lock['package']]
if packages:
packages = packages.split(',')
Copy link
Member

Choose a reason for hiding this comment

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

Better do parsing earlier in the flow, ideally in main(), and pass packages here as a list of strings.

if packages:
packages = packages.split(',')
logging.debug('Considering only packages: %s', packages)
all_packages = {p['name']: p for p in cargo_lock['package']}
Copy link
Member

Choose a reason for hiding this comment

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

The all_packages dict feels out of place here, since it's only used within packages_dependencies() scope. Better pass the whole cargo_lock to packages_dependencies(), and there get whatever information is needed from it.

Comment on lines -312 to +334
else:
pkg_sources, cargo_vendored_entry = pkg
pkg_sources, cargo_vendored_entry = pkg
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: please make unrelated changes in separate commits.

@@ -338,6 +359,7 @@ async def generate_sources(cargo_lock, git_tarballs=False):
def main():
parser = argparse.ArgumentParser()
parser.add_argument('cargo_lock', help='Path to the Cargo.lock file')
parser.add_argument('-p', '--packages', help='Comma-separated packages in Cargo.lock to build')
Copy link
Member

Choose a reason for hiding this comment

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

This help text is a bit misleading, since we don't control what Cargo builds. Maybe "...list of package name to fetch" would be better?

@hfiguiere hfiguiere added the cargo Rust cargo label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cargo Rust cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants