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

cosmic-packages: move to new cargo fetcher #356539

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

JohnRTitor
Copy link
Contributor

@JohnRTitor JohnRTitor commented Nov 16, 2024

Manually did it by hand. Though we should probably consider creating a script to get rid of other excess Cargo.locks.

Originally intended as part of #356385 but split to make reviews easier

CC: #327063

Closes #356821

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
  • 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@a-kenji
Copy link
Contributor

a-kenji commented Nov 16, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 356539


x86_64-linux

✅ 20 packages built:
  • cosmic-applibrary
  • cosmic-bg
  • cosmic-comp
  • cosmic-comp.debug
  • cosmic-design-demo
  • cosmic-edit
  • cosmic-files
  • cosmic-greeter
  • cosmic-launcher
  • cosmic-notifications
  • cosmic-osd
  • cosmic-panel
  • cosmic-randr
  • cosmic-session
  • cosmic-settings
  • cosmic-settings-daemon
  • cosmic-store
  • cosmic-term
  • cosmic-workspaces-epoch
  • cosmic-workspaces-epoch.debug

Copy link
Contributor

@a-kenji a-kenji left a comment

Choose a reason for hiding this comment

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

Thank you for splitting this in to reviewable chunks.

I have not tried every functionality of the packages in here, but ran every output against master to see if they match up.

For programs like cosmic-edit I tried them out manually.

@JohnRTitor
Copy link
Contributor Author

@ofborg build cosmic-applibrary cosmic-bg cosmic-comp cosmic-design-demo cosmic-edit cosmic-files cosmic-greeter cosmic-launcher cosmic-notifications cosmic-osd cosmic-panel cosmic-randr cosmic-session cosmic-settings cosmic-settings-daemon cosmic-store cosmic-term cosmic-workspaces-epoch

@TomaSajt
Copy link
Contributor

TomaSajt commented Nov 17, 2024

There still seems to be an issue with git fetching, maybe submodule related.

You can see in the logs that the call to nix-prefetch-git caused the build faliure.

Seeing a lot of http 403 codes.

Originally git repos were fetched sequentially outside of the multiprocessing part. Maybe I should restore the logic to be like that.

@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 17, 2024

Originally git repos were fetched sequentially outside of the multiprocessing part.

Perhaps it should respect enableParallelBuilding?

EDIT: enableParallelBuilding may not mean parallelFetching, I suppose

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Nov 17, 2024
@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Nov 17, 2024
@nix-owners nix-owners bot requested review from figsoda, winterqt and zowoq November 17, 2024 21:18
@TomaSajt TomaSajt force-pushed the cosmic-migrate-to-new-fetcher branch from dbdafbd to ff707e5 Compare November 17, 2024 21:28
@TomaSajt TomaSajt force-pushed the cosmic-migrate-to-new-fetcher branch from ff707e5 to 9fd5a8d Compare November 17, 2024 22:11
@TomaSajt
Copy link
Contributor

Okay, seems like decreasing the concurrency limit works for OfBorg. I didn't encounter issues locally, even when trying to build all cargoDeps at the same time. Opened a separate PR and cherry picked its commit into this PR.

@TomaSajt
Copy link
Contributor

I also saw that #356743 added some retry logic, maybe that could also be incorporated later.

@ahoneybun
Copy link
Contributor

@JohnRTitor does this change stop us from using nix-update to update our packaging?

@JohnRTitor
Copy link
Contributor Author

@ofborg build cosmic-applibrary cosmic-bg cosmic-comp cosmic-design-demo cosmic-edit cosmic-files cosmic-greeter cosmic-launcher cosmic-notifications cosmic-osd cosmic-panel cosmic-randr cosmic-session cosmic-settings cosmic-settings-daemon cosmic-store cosmic-term cosmic-workspaces-epoch

@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 18, 2024

It won't "stop" you, nix update just isn't compatible with it since it's a new fetcher. See Mic92/nix-update#298, adding support should be one PR away

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Nov 18, 2024
@JohnRTitor JohnRTitor merged commit 99268a0 into NixOS:master Nov 18, 2024
32 of 34 checks passed
@JohnRTitor JohnRTitor deleted the cosmic-migrate-to-new-fetcher branch November 18, 2024 12:09
@SuperSandro2000
Copy link
Member

That tastes good :P

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: rust 8.has: clean-up 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants