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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding sparse protocol #304583

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

Conversation

rayanpiro
Copy link

@rayanpiro rayanpiro commented Apr 16, 2024

Description of changes

Adding new sparse registry protocol to rust builders with minimal changes.
Properly building fetching from kellnr self hosted instance.

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@marcusramberg
Copy link
Contributor

Commit message should be updated to follow the commit conventions.

Copy link
Member

@winterqt winterqt left a comment

Choose a reason for hiding this comment

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

Overall, this looks good, thank you so much! I haven't been keeping up with the progress of the sparse registry -- I had thought that it wouldn't actually change the lock file, but looks like I'm wrong there.

Could you add a small test for it? See pkgs/build-support/rust/test/import-cargo-lock. My first instinct would be to just add it to basic, but depending on how things work with lockfile generation, might be best just to add a new test.

Commit message should be updated to follow the commit conventions.

Beat me to it, thanks! +1, please do this.

pkgs/build-support/rust/import-cargo-lock.nix Outdated Show resolved Hide resolved
@rayanpiro rayanpiro force-pushed the sparse-protocol branch 2 times, most recently from 06a7494 to 49b0780 Compare April 17, 2024 07:15
@rayanpiro
Copy link
Author

rayanpiro commented Apr 17, 2024

Changed commit to follow the conventions.
I'll try to review how tests are done and add a new one or modify the basic one today or tomorrow. It's not a big difference, just Cargo.lock with sparse+https:// instead of registry+https:// but I'll review it first and post if any doubt.

Overall, this looks good, thank you so much! I haven't been keeping up with the progress of the sparse registry -- I had thought that it wouldn't actually change the lock file, but looks like I'm wrong there.

That's true for crates.io, but if you want to use a self hosted alternative like kellnr isn't working with this. You have to define the registry as sparse+https://... and every dependency on Cargo.lock will be the same.

When added the new sparse protocol the derivation is not handling properly Cargo.lock with sparse+ registries
@rayanpiro
Copy link
Author

rayanpiro commented Apr 17, 2024

Ok, some modifications were made looking for:

  • Add support for download URL because could be or not the same than index URL.
  • Modify less amount of code as possible.
  • Add a basic sparse test as solicited.

Hope everything seems good enough now.

@rayanpiro rayanpiro closed this Apr 17, 2024
@rayanpiro rayanpiro reopened this Apr 17, 2024
@rayanpiro rayanpiro requested a review from winterqt April 18, 2024 08:51
@rayanpiro
Copy link
Author

@marcusramberg @winterqt can you please review the latest changes?

@winterqt
Copy link
Member

winterqt commented May 3, 2024

I'll take a look in a few hours, thanks for re-pinging.

Copy link
Contributor

@marcusramberg marcusramberg left a comment

Choose a reason for hiding this comment

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

lgtm

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.

None yet

4 participants