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.

Sign up for GitHub

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

Jump to bottom

crusader: init at 0.3.2 #374879

Merged
merged 1 commit into from
Jan 20, 2025
Merged

crusader: init at 0.3.2 #374879

merged 1 commit into from
Jan 20, 2025
+90 −0

Conversation

x123
Copy link
Contributor

@x123 x123 commented Jan 18, 2025

Add rust-based crusader network throughput and latency testing tool.

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

@x123 x123 force-pushed the x123-init-crusader branch from 69af2dd to 4fbe8dd Compare January 18, 2025 20:07
@x123 x123 marked this pull request as ready for review January 18, 2025 20:08
lucasew
lucasew reviewed Jan 19, 2025
View reviewed changes
pkgs/by-name/cr/crusader/package.nix Outdated
src = fetchFromGitHub {
owner = "Zoxc";
repo = "crusader";
rev = "v${version}";
Copy link
Contributor

Choose a reason for hiding this comment

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

rev -> tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I always seem to forgot this.

@lucasew
Copy link
Contributor

lucasew commented Jan 19, 2025

Please use fetchCargoDeps

Example: https://github.com/NixOS/nixpkgs/pull/364125/files#diff-c143dc661373803583ca5fe448400037ae055bf6d2f75ef71f7a5cda16cb1b3cR29

@x123 x123 force-pushed the x123-init-crusader branch from 4fbe8dd to 83ed156 Compare January 19, 2025 08:54
@x123
Copy link
Contributor Author

x123 commented Jan 19, 2025

Please use fetchCargoDeps

Example: https://github.com/NixOS/nixpkgs/pull/364125/files#diff-c143dc661373803583ca5fe448400037ae055bf6d2f75ef71f7a5cda16cb1b3cR29

This has also been fixed. Thanks for the feedback.

@x123 x123 force-pushed the x123-init-crusader branch from 83ed156 to ced6f69 Compare January 19, 2025 11:41
lucasew
lucasew reviewed Jan 19, 2025
View reviewed changes
pkgs/by-name/cr/crusader/package.nix Outdated

sourceRoot = "${src.name}/src";

cargoDeps = rustPlatform.fetchCargoVendor {
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC there is an argument that you can only specify a hash of the cargo deps, I don't remember the name tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that what I'm doing by specifying hash to rustPlatform.fetchCargoVendor?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but there is an argument where you just pass the hash and it becomes equivalent to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, I changed it to the same style I see almost everywhere else:

  useFetchCargoVendor = true;
  cargoHash = "sha256-f0TWiRX203/gNsa9UEr/1Bv+kUxLAK/Zlw+S693xZlE=";

@lucasew
Copy link
Contributor

lucasew commented Jan 19, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374879

x86_64-linux

✅ 1 package built:
  • crusader

@x123 x123 force-pushed the x123-init-crusader branch 2 times, most recently from fc2da95 to 0eb5c58 Compare January 19, 2025 14:01
@lucasew
Copy link
Contributor

lucasew commented Jan 19, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374879

x86_64-linux

✅ 1 package built:
  • crusader

@x123 x123 force-pushed the x123-init-crusader branch from 0eb5c58 to 460a86b Compare January 19, 2025 18:16
lucasew
lucasew approved these changes Jan 19, 2025
View reviewed changes
Copy link
Contributor

@lucasew lucasew left a comment

Choose a reason for hiding this comment

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

Diff LGTM. I didn't test locally tho.

@lucasew
Copy link
Contributor

lucasew commented Jan 19, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374879

x86_64-linux

✅ 1 package built:
  • crusader

@lucasew lucasew requested a review from GaetanLepage January 19, 2025 18:40
GaetanLepage
GaetanLepage requested changes Jan 19, 2025
View reviewed changes
pkgs/by-name/cr/crusader/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/crusader/package.nix Outdated Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 20, 2025
@x123 x123 force-pushed the x123-init-crusader branch from 460a86b to d5d1843 Compare January 20, 2025 08:34
@x123 x123 requested a review from GaetanLepage January 20, 2025 08:43
@x123 x123 force-pushed the x123-init-crusader branch 2 times, most recently from f08d458 to fe78caa Compare January 20, 2025 09:09
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374879

x86_64-linux

✅ 1 package built:
  • crusader

aarch64-linux

✅ 1 package built:
  • crusader

x86_64-darwin

✅ 1 package built:
  • crusader

aarch64-darwin

✅ 1 package built:
  • crusader

GaetanLepage
GaetanLepage requested changes Jan 20, 2025
View reviewed changes
pkgs/by-name/cr/crusader/package.nix Show resolved Hide resolved
pkgs/by-name/cr/crusader/package.nix Outdated Show resolved Hide resolved
@x123 x123 force-pushed the x123-init-crusader branch from fe78caa to 0d4c0f7 Compare January 20, 2025 09:30
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374879

x86_64-linux

✅ 1 package built:
  • crusader

aarch64-linux

✅ 1 package built:
  • crusader

x86_64-darwin

✅ 1 package built:
  • crusader

aarch64-darwin

✅ 1 package built:
  • crusader

@GaetanLepage
Copy link
Contributor

It probably needs extra dependencies for wayland:

❯ ./crusader-x86_64-linux/bin/crusader-gui
Failed to run GUI: winit EventLoopError: os error at /build/crusader-0.3.2-vendor/winit-0.29.15/src/platform_impl/linux/wayland/event_loop/mod.rs:80: The wayland library could not be loaded

@x123
Copy link
Contributor Author

x123 commented Jan 20, 2025

It probably needs extra dependencies for wayland:

❯ ./crusader-x86_64-linux/bin/crusader-gui
Failed to run GUI: winit EventLoopError: os error at /build/crusader-0.3.2-vendor/winit-0.29.15/src/platform_impl/linux/wayland/event_loop/mod.rs:80: The wayland library could not be loaded

I don't really have a good system with wayland for testing. Usually an strace of that binary will reveal the libs it's missing. Any chance you could suggest an additional commit that corrects the issue?

@GaetanLepage
Copy link
Contributor

Sure, here it is: https://paste.glepage.com/raw/ape-pigeon-wasp

@GaetanLepage
Copy link
Contributor

Maybe just add wayland to buildInputs (if running on linux).

@x123 x123 force-pushed the x123-init-crusader branch from 0d4c0f7 to 71323f3 Compare January 20, 2025 11:33
@x123
Copy link
Contributor Author

x123 commented Jan 20, 2025

Maybe just add wayland to buildInputs (if running on linux).

I've just added wayland. Maybe you can test crusader-gui again? Thanks.

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374879

x86_64-linux

✅ 1 package built:
  • crusader

aarch64-linux

✅ 1 package built:
  • crusader

x86_64-darwin

✅ 1 package built:
  • crusader

aarch64-darwin

✅ 1 package built:
  • crusader

@GaetanLepage
Copy link
Contributor

I've just added wayland. Maybe you can test crusader-gui again? Thanks.

It was still giving the same error as the lib is loaded at runtime.

I took the initiative of fixing it and force-pushing.

@GaetanLepage
Copy link
Contributor

image

@GaetanLepage GaetanLepage requested a review from drupol January 20, 2025 13:11
@GaetanLepage GaetanLepage merged commit bc2a7ec into NixOS:master Jan 20, 2025
25 of 27 checks passed
@GaetanLepage
Copy link
Contributor

Thanks @x123 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GaetanLepage GaetanLepage GaetanLepage approved these changes

@lucasew lucasew lucasew approved these changes

@drupol drupol drupol approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@x123 @lucasew @GaetanLepage @drupol @wegank