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

treewide: use new cargo fetcher #356862

Closed
wants to merge 214 commits into from
Closed

Conversation

Bot-wxt1221
Copy link
Member

@Bot-wxt1221 Bot-wxt1221 commented Nov 18, 2024

Already done. We should run nixpkgs-review to check if some package have different Cargo.lock when building and in nixpkgs, like veloren.

I have writen a script to update it automatically.

Now It can solve:

cargoLock = {
  lockFile = xxx;
  outputhahes = {xxx};
};
cargoLock.lockFile = xxx;
cargoLock.outputHashes = {xxx};

Script: https://github.com/Bot-wxt1221/cargo-rename

Usage:

Compile with gcc. Make sure fetch-cargo.py is in the same path. Exec with a xx/pkgs/by-name/xx/xx/package.nix

cc #327063

#349360

Step to reduce:

  1. Generate a file list with cargoLock:
rg "cargoLock" --files-with-matches > filewithcargoLock

cat filewithcargoLock | rev | cut -d / -f 2|rev > packagename
  1. run update-all

useFetchCargoVendor

  • Squash all commit

  • 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/)

  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@Bot-wxt1221 Bot-wxt1221 marked this pull request as draft November 18, 2024 03:58
@Bot-wxt1221 Bot-wxt1221 changed the title [WIP]tree-wide: use new cargo fetcher tree-wide: use new cargo fetcher Nov 18, 2024
@Bot-wxt1221
Copy link
Member Author

@ofborg build libconfig hocon xdg-desktop-portal-cosmic

Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

See above

@Bot-wxt1221
Copy link
Member Author

@JohnRTitor We really need a quicker fetcher. I've found that it would be OK if we set max-retries time to 10. And allow run 100 threads in one time.

@JohnRTitor
Copy link
Contributor

Unfortunately more concurrent jobs will just seen as a bot spam by GitHub and other servers, so the requests will error out (403).

This was the motivation behind 9fd5a8d as Ofborg wasn't able to fetch the git dependencies properly hitting 403.

@Bot-wxt1221
Copy link
Member Author

Bot-wxt1221 commented Nov 18, 2024

@JohnRTitor I don't mean we should allow it run at 100 or similar thread. But I find that if we allow it retry for 10 times, even though I run 100 thread, I doesn't meet error when running script.

@JohnRTitor
Copy link
Contributor

if we set max-retries time to 10

CC @TomaSajt, could this fix the issues encountered in #356385 (comment)?

@TomaSajt
Copy link
Contributor

I wouldn't be against a retry system, but I don't think we should up the thread numbers by much.

Speed was not really a concern for me, since you're only going to build the FOD once or twice in normal circumstances.

@TomaSajt
Copy link
Contributor

Looks like the current script can't handle cases where upstream doesn't have the lockfile. This means that we can't just blindly assume that having outputHashes present means that it can be migrated.

@Bot-wxt1221
Copy link
Member Author

Bot-wxt1221 commented Nov 18, 2024

We can run nixpkgs-review to check what package can't be built and remove them.

@TomaSajt
Copy link
Contributor

@Tomahna We can run nixpkgs-review to check what package can't be built and remove them.

Probably pinged the wrong person.

Also, these would probably result in eval errors.
If you look at the nixpkgs-vet CI jobs you can see which packages fail.

@Bot-wxt1221
Copy link
Member Author

Bot-wxt1221 commented Nov 18, 2024

@TomaSajt Actually still 50 package are planned to move. So I'll do it after it. And that's the reason why it is draft.

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Nov 18, 2024
@Bot-wxt1221 Bot-wxt1221 force-pushed the cargo branch 3 times, most recently from 3731e45 to 6532c31 Compare November 19, 2024 03:43
@Bot-wxt1221 Bot-wxt1221 marked this pull request as ready for review November 19, 2024 04:09
@Bot-wxt1221
Copy link
Member Author

The path should be different now. But not really bad, I'm running nixpkgs-review.

@JohnRTitor
Copy link
Contributor

Could you seperate into 6905f1e another PR?

This way we can just squash merge this PR, without having you squash the commits yourself.

Or you can just squash all commits except that one and force push.

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Mixing substantive changes and formatting changes in the same commits in a treewide PR this huge is not acceptable.

Also, treewide replacements of opaque FOD hashes are among the most dangerous changes that can be made in Nixpkgs, given the existence of FOD injection attacks; just building packages doesn’t avoid that risk. I’d really like to see someone do an automated comparison of the resulting vendor directories, or at least write a script that can locally reproduce the hashes here.

Edit: Okay, it seems like the steps in the PR description may be sufficient to reproduce these commits? In that case I would really like to see someone do a full review of the script’s source code and confirm that they obtained the exact same tree after running it from a pristine state. This is the kind of change that would make it extremely easy to insert backdoors into Nixpkgs.

@@ -1,4 +1,18 @@
{ lib, fetchFromGitHub, rustPlatform, alsa-lib, atk, cairo, dbus, gdk-pixbuf, glib, gtk3, pango, pkg-config, makeDesktopItem }:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not include formatting changes in this PR.

Keep only the new fetcher related changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Look above.

@TomaSajt
Copy link
Contributor

To be honest, I'm also more in favor of multiple smaller PRs. We're not rushing anywhere.
It would also be much easier to review.

@Bot-wxt1221
Copy link
Member Author

Bot-wxt1221 commented Nov 19, 2024

About nixfmt:

I can split the nixfmt commit into one.

About reduce:

The script was in https://github.com/Bot-wxt1221/cargo-rename. Anyone can reduce it.

About split into smaller PRs:

It won't make it more easier. We still need to recompile every package by hand or by nixpkgs-review in smaller PR.

I will provide any thing to reduce this PR.

@TomaSajt @JohnRTitor @emilazy

@Mic92
Copy link
Member

Mic92 commented Nov 19, 2024

@Bot-wxt1221 Maybe you can do a pull request chain? I.e just branch off every after X commits and make a new pull request depending on the previous one? That hopefully is an acceptable management overhead for both sides. Maybe 20 packages per pull request could work, which would be 10 pull requests.

@Bot-wxt1221
Copy link
Member Author

Will split into multi PR. I will open an issue to track it.

@Bot-wxt1221
Copy link
Member Author

#357257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: emacs Text editor 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 6.topic: rust 6.topic: vim
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants