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

packetry: init at 0.3.0 #351392

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

Conversation

carlossless
Copy link
Contributor

@carlossless carlossless commented Oct 26, 2024

A fast, intuitive USB 2.0 protocol analysis application for use with Cynthion. https://github.com/greatscottgadgets/packetry

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.

Related to #81418

Copy link
Contributor

@zi3m5f zi3m5f left a comment

Choose a reason for hiding this comment

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

Is there a reason you are opening your third PR for the same package?

If it is for the version change please just change PR and commit.

pkgs/by-name/pa/packetry/package.nix Outdated Show resolved Hide resolved
@carlossless
Copy link
Contributor Author

carlossless commented Oct 26, 2024

@zi3m5f

Is there a reason you are opening your third PR for the same package?

I figured that would be more appropriate than bumping the existing pr without changing the version reference in my branch, but I could just name it with an agnostic name.

If it is for the version change please just change PR and commit.

Sure, will do so in the future.

Copy link
Contributor

@zi3m5f zi3m5f left a comment

Choose a reason for hiding this comment

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

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 351392


x86_64-linux

✅ 1 package built:
  • packetry

pkgs/by-name/pa/packetry/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/packetry/package.nix Show resolved Hide resolved

# Tests need a display to run
doCheck = false;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
postInstall = lib.optionalString (!hostPlatform.isWindows) ''
rm $out/bin/packetry-cli
'';

packetry-cli is only needed on windows atm: greatscottgadgets/packetry#154
So we could remove it. Not tested.

The check for windows is not needed so long as meta.platforms doesn't include it but better be safe than sorry?
Upstream builds for windows but I can't test it so I will not suggest adding it to meta.platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this, but I am a little conflicted about adding the hostPlatform check when windows is not part of the supported platforms (in this package.nix).

I am also unaware of how to approach testing the build on windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of the check a comment about it being needed only on windows with a link to the pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I opted to do that instead.

@carlossless carlossless force-pushed the packages/packetry-0.3.0 branch 2 times, most recently from f9a2e0b to ddfa597 Compare October 27, 2024 17:35
Copy link
Contributor

@zi3m5f zi3m5f left a comment

Choose a reason for hiding this comment

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

LGTM: builds and gui starts.

Further room for improvement but not necessary right now:

@carlossless: could you mark the code comments as resolved (minus preCheck), thanks

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.

2 participants