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

Automatically filter out empty directories in cleanCargoSource #725

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rijkvp
Copy link

@rijkvp rijkvp commented Oct 23, 2024

Motivation

Currently, the source filtering done by cleanCargoSource causes a lot of empty directories to be copied into the build when all the files in them are filtered out. For example, I noticed that the /target directory is copied by default, but it only contains other empty directories. This PR filters out all empty directories in cleanCargoSource to avoid unnecessary rebuilds.

Checklist

  • added tests to verify new behavior
  • added an example template or updated an existing one
  • updated docs/API.md (or general documentation) with changes
  • updated CHANGELOG.md

@ipetkov
Copy link
Owner

ipetkov commented Oct 24, 2024

Hi @rijkvp thanks for the contribution!

One thing worth mentioning is that I've noticed that filesets only work with actual local file paths (e.g. ./some/path) but aren't as "general" as source filters (which can operate on other fixed point derivations). Take a look at #726 which adds an explicit regression test for this.

If we were to keep cleanCargoSource backwards compatible (i.e. a wrapper around lib.cleanSourceWith) but introduce a new cleanCargoFileset with your proposed implementation I'd be happy to consider merging it in!

@rijkvp
Copy link
Author

rijkvp commented Oct 24, 2024

Good catch @ipetkov! I didn't notice this would break that.
I agree that adding a new function using file sets is a better idea, and I am happy to work on it.

I'm imagining something like this:

craneLib.buildPackage {
    src = craneLib.makeCargoFileset [
        ./assets
        ./LICENSE
        (lib.fileset.fileFilter (file: file.hasExt "md") ./docs)
        # ... any additional file sets
    ] ./.;
    # ...
}

Which wraps something like filterCargoFileset which is kind of similar to filterCargoSources, which could be used to further customize the sources:

craneLib.buildPackage {
    src = lib.fileset.toSource {
        root = ./.;
        fileset = lib.fileset.unions [
            craneLib.filterCargoFileset # includes Cargo.toml, Cargo.lock, *.rs, etc.
            # additional file sets are included here
        ];
    };
    # ...
}

I'd love to hear your opinion on this. Also I'm not sure about the naming.

@ipetkov
Copy link
Owner

ipetkov commented Oct 27, 2024

@rijkvp two thoughts come to mind:

  1. regarding naming: maybe we can scope it under fileset to "match" nixpkgs? e.g. craneLib.fileset.cargoFileset seems sensible to me
  2. regarding src = craneLib.makeCargoFileset ...: I know that filesets have to explicitly be converted to a source via lib.fileset.toSource, so perhaps we should not have craneLib.fileset.cargoFileset also call lib.fileset.toSource. That way if someone wants to keep tweaking the final fileset they can keep working with fileset objects (we can always introduce another helper in the future if we find it too noisy)

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.

2 participants