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

rustPlatform.fetchCargoVendor: init #349360

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

Conversation

TomaSajt
Copy link
Contributor

@TomaSajt TomaSajt commented Oct 17, 2024

This PR adds an alternative method to vendor the dependencies of a cargo project.
It's currently implemented in python, but could be changed later, if need be.

I converted some packages to use this fetcher as an example.

If you can come up with a good name for this fetcher, please share it below.

Using this fetcher, we won't have to worry about cargo changing its vendoring implementation, thus we can replace all importCargoLock usages where it was only required because of the presence of git dependencies.

The main idea is that we try to create a minimal staging FOD.
We fetch the tarballs into $out/tarballs/${name}-${version}.tar.gz and we fetch the git repos to $out/git/${git_sha_rev}.

This is the most important part to get right, since we will only be able to minimally change this in the future.

This staging FOD will be used to create the actual vendor dir.

Since this no longer requires network access, we can freely change the implementation of this part without worrying about that hash.

The cargo workspace inheritance logic was already reimplemented by a script written for importCargoLock, so I just reused that.


Unfortunately, this way the output of this new fetcher will not directly be a FOD.
The FOD itself will be accessible from cargoDeps.vendorStaging.

TODO: maybe add an easy way to override the FOD inputs.


Currently there is no way to use this through buildCargoPackage, since it hard-codes the usage of fetchCargoTarball.
Should there be a toggle to use this fetcher? customFetcher = true;
Maybe an enum? cargoFetcher = "custom" vs cargoFetcher = "default";
Maybe just cargoFetcher = rustPlatform.fetchCargoVendor


The implementation could be easily changed so that the fetcher will not need the entire src passed in, only a path to the Cargo.lock. This isn't IFD, since we only need the path. This would be similar to how it's done with fetchYarnDeps.
Though, having it as a normal mkDerivation, like it is now, we can also apply patches and use hooks.


It would be possible to skip the staging phase and make the vendor dir the FOD, however, this would lock our output format to never be able to change again.


The current implementation uses nix-prefetch-git to fetch a reproducible git tree, as I wanted to make sure I didn't make any implementation mistakes. I wonder if there's a better method than this...


Nothing is stopping us from also compressing the final vendor directory into a tarball, just like with fetchCargoTarball.
Tarballs - obviously - take up less space, so ideally we would do this.
Though a better fetcher name might be needed in that case.
I'm keeping it as outputting a directory until the testing is done.


Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@TomaSajt TomaSajt force-pushed the new-cargo-vendor-fetcher branch 2 times, most recently from 8f86bba to 8581797 Compare October 17, 2024 21:00
pkgs/build-support/rust/mk-cargo-vendor-deps-script.py Outdated Show resolved Hide resolved
pkgs/build-support/rust/mk-cargo-vendor-deps-script.py Outdated Show resolved Hide resolved
pkgs/build-support/rust/mk-cargo-vendor-deps-script.py Outdated Show resolved Hide resolved
pkgs/build-support/rust/mk-cargo-vendor-deps-script.py Outdated Show resolved Hide resolved
pkgs/build-support/rust/mk-cargo-vendor-deps-script.py Outdated Show resolved Hide resolved
pkgs/build-support/rust/mk-cargo-vendor-deps-script.py Outdated Show resolved Hide resolved
pkgs/build-support/rust/mk-cargo-vendor-deps-script.py Outdated Show resolved Hide resolved

# TODO: support other repositories
# or maybe allow getting downloadUrl through an API request
def download_tarball(args: tuple[dict, str]):
Copy link
Member

Choose a reason for hiding this comment

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

A TypeDict would provide more insight on what keys the dictionary has

pkgs/build-support/rust/mk-cargo-vendor-deps-script.py Outdated Show resolved Hide resolved
@TomaSajt TomaSajt force-pushed the new-cargo-vendor-fetcher branch 3 times, most recently from 143eb2c to eac2cca Compare October 17, 2024 21:33
@ofborg ofborg bot requested a review from lucastso10 October 17, 2024 23:54
@ofborg ofborg bot requested review from figsoda and winterqt October 18, 2024 11:24
@TomaSajt TomaSajt changed the title WIP: rustPlatform.mkCargoVendorDeps: init WIP: rustPlatform.fetchCargoVendor: init Oct 18, 2024
@TomaSajt TomaSajt changed the title WIP: rustPlatform.fetchCargoVendor: init rustPlatform.fetchCargoVendor: init Oct 18, 2024
@TomaSajt TomaSajt marked this pull request as ready for review October 18, 2024 14:46
@nix-owners nix-owners bot requested review from Mic92 and zowoq October 18, 2024 14:47
@TomaSajt
Copy link
Contributor Author

@ofborg build granian popsicle

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Oct 18, 2024

I wonder why ofborg is failing...
Edit: looks like #331167 hit master, so I need config.toml instead of config.

@TomaSajt TomaSajt force-pushed the new-cargo-vendor-fetcher branch 2 times, most recently from 6533da7 to 260c771 Compare October 18, 2024 21:47
@TomaSajt
Copy link
Contributor Author

@ofborg build granian popsicle

@Sigmanificient
Copy link
Member

Sigmanificient commented Oct 19, 2024

@Mic92 For typing dictionnaries, I proposed to use typedDict above, which would provide way better typing than having an Any as value type

elif method == "create-vendor":
create_vendor(vendor_staging_dir=sys.argv[2], out_dir=sys.argv[3])
else:
raise Exception(f"Unknown method: {method}")
Copy link
Member

@Mic92 Mic92 Oct 19, 2024

Choose a reason for hiding this comment

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

I would make create your own Exception at the start of the script:

class Error(Exception):
    pass

and than catch this exception in the main method:

try:
    ...
exception Error as e:
   eprint(e)

This won't show than error stacktraces for our own errors, which is a bit more user friendly (because users often don't find the relevant error from a stack trace).

fetchCargoVendorUtil
cargo
replaceWorkspaceValues
];
Copy link
Member

@Mic92 Mic92 Oct 19, 2024

Choose a reason for hiding this comment

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

Would be nice to also type check this:

Suggested change
];
];
passthru.tests = {
typecheckAndLint = runCommand "mypy-check" { nativeBuildInputs = [ mypy ruff ]; } ''
mypy ${./.}
ruff format --check ${./.}
ruff check ${./.}
touch $out
'';
};

@Mic92
Copy link
Member

Mic92 commented Oct 19, 2024

@Mic92 For typing dictionnaries, I proposed to use typedDict above, which would provide way better typing than having an Any as value type

ok with me. Does typedDict has to include all fields or only the ones we care about?

@Mic92
Copy link
Member

Mic92 commented Oct 19, 2024

If this can replace our vendored cargo.lock files in many pacles, this is a great addition. I hope we can consolidate this and replace our current vendoring. But would be also fine to do this as a second step and incrementally convert our usage to the new fetcher.

@Mic92
Copy link
Member

Mic92 commented Oct 19, 2024

What about using black --max-length 79 to ensure pep8 compliance & flake8 E501 rule? It would also make the diff easier to read in split mode (as it is ~90 chars)

I think we can just use ruff format and ruff check. Pretty much the same thing but also more potential lints and also faster.

@Sigmanificient
Copy link
Member

Sigmanificient commented Oct 19, 2024

@Mic92 For typing dictionnaries, I proposed to use typedDict above, which would provide way better typing than having an Any as value type

ok with me. Does typedDict has to include all fields or only the ones we care about?

you can set total=False on the class to hint that the typing only has the relevant keys

@Sigmanificient
Copy link
Member

What about using black --max-length 79 to ensure pep8 compliance & flake8 E501 rule? It would also make the diff easier to read in split mode (as it is ~90 chars)

I think we can just use ruff format and ruff check. Pretty much the same thing but also more potential lints and also faster.

Yeah, i suggested black because i've been using it for years ahah, has long has it complies with pep8 i believe it's fine

@Mic92
Copy link
Member

Mic92 commented Oct 19, 2024

What about using black --max-length 79 to ensure pep8 compliance & flake8 E501 rule? It would also make the diff easier to read in split mode (as it is ~90 chars)

I think we can just use ruff format and ruff check. Pretty much the same thing but also more potential lints and also faster.

Yeah, i suggested black because i've been using it for years ahah, has long has it complies with pep8 i believe it's fine

ruff format should be almost 100% compatible to black.

@TomaSajt TomaSajt force-pushed the new-cargo-vendor-fetcher branch 2 times, most recently from 594e710 to c6fb7f7 Compare October 19, 2024 17:21
@TomaSajt TomaSajt force-pushed the new-cargo-vendor-fetcher branch 3 times, most recently from ebde301 to 756989e Compare October 19, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants