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

packetry: init at 0.4.0 #351392

Merged
merged 1 commit into from
Nov 9, 2024
Merged

packetry: init at 0.4.0 #351392

merged 1 commit into from
Nov 9, 2024

Conversation

carlossless
Copy link
Contributor

@carlossless carlossless commented Oct 26, 2024 •
edited
Loading

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

@carlossless carlossless mentioned this pull request Oct 26, 2024
Closed
13 tasks
@carlossless carlossless requested review from afh and fabaff October 26, 2024 09:24
zi3m5f
zi3m5f reviewed Oct 26, 2024
View reviewed changes
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 •
edited
Loading

@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.

@carlossless carlossless force-pushed the packages/packetry-0.3.0 branch from 347def5 to f221ee0 Compare October 26, 2024 13:58
@carlossless carlossless requested a review from zi3m5f October 26, 2024 14:02
zi3m5f
zi3m5f suggested changes Oct 27, 2024
View reviewed changes
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
pkgs/by-name/pa/packetry/package.nix

# 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
zi3m5f
zi3m5f approved these changes Oct 27, 2024
View reviewed changes
Copy link
Contributor

@zi3m5f zi3m5f left a comment •
edited
Loading

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

@carlossless carlossless force-pushed the packages/packetry-0.3.0 branch from e13ef86 to 7d50137 Compare October 27, 2024 23:00
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4793

@carlossless carlossless force-pushed the packages/packetry-0.3.0 branch from 7d50137 to 745b82d Compare November 3, 2024 15:31
@carlossless carlossless changed the title packetry: init at 0.3.0 packetry: init at 0.4.0 Nov 3, 2024
@carlossless
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 351392

x86_64-linux

✅ 1 package built:
  • packetry

aarch64-linux

✅ 1 package built:
  • packetry

x86_64-darwin

✅ 1 package built:
  • packetry

aarch64-darwin

✅ 1 package built:
  • packetry

@carlossless carlossless force-pushed the packages/packetry-0.3.0 branch from 745b82d to 0abe065 Compare November 8, 2024 17:29
@carlossless carlossless force-pushed the packages/packetry-0.3.0 branch from 0abe065 to d051f42 Compare November 8, 2024 19:07
@carlossless
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 351392

x86_64-linux

✅ 1 package built:
  • packetry

aarch64-linux

✅ 1 package built:
  • packetry

x86_64-darwin

✅ 1 package built:
  • packetry

aarch64-darwin

✅ 1 package built:
  • packetry

@khaneliman
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 351392

x86_64-linux

✅ 1 package built:
  • packetry

aarch64-linux

✅ 1 package built:
  • packetry

x86_64-darwin

✅ 1 package built:
  • packetry

aarch64-darwin

✅ 1 package built:
  • packetry

khaneliman
khaneliman approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@khaneliman khaneliman left a comment

Choose a reason for hiding this comment

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

LGTM

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 8, 2024
@khaneliman khaneliman merged commit d79447f into NixOS:master Nov 9, 2024
28 of 29 checks passed
@carlossless carlossless deleted the packages/packetry-0.3.0 branch November 9, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zi3m5f zi3m5f zi3m5f approved these changes

@khaneliman khaneliman khaneliman approved these changes

@afh afh Awaiting requested review from afh

@fabaff fabaff Awaiting requested review from fabaff

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 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
@carlossless @nixos-discourse @khaneliman @zi3m5f @wegank