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

Clarissa: init at 1.0-80-g00c2581 #69660

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Clarissa: init at 1.0-80-g00c2581 #69660

wants to merge 5 commits into from

Conversation

evils
Copy link
Member

@evils evils commented Sep 27, 2019

Motivation for this change

I want to share my tool.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
    • 36910136
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@infinisil
Copy link
Member

I'm not sure if we want to include tools in nixpkgs that probably just have a single user, since there's a non-zero cost to including new packages, namely maintenance, hydra build time and for NixOS modules it even slows down everybody's nixos-rebuild. See also the discussion in https://discourse.nixos.org/t/we-need-more-defined-guidelines-for-package-inclusion/3592

Maybe NUR is a better place to put such packages, or eventually as Flakes.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

references to pkgs should be unnecessary

cc @infinisil for his module knowledge

EDIT: you already commented, lol

pkgs/tools/networking/clarissa/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/clarissa/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/clarissa/default.nix Outdated Show resolved Hide resolved
@evils
Copy link
Member Author

evils commented Feb 29, 2024

reopened in honour of the 1st anniversary of the v1 release 4 years ago

@evils evils changed the title Clarissa: init at v0.6 Clarissa: init at 1.0-73-g9ea32f0 Feb 29, 2024
@evils evils changed the title Clarissa: init at 1.0-73-g9ea32f0 Clarissa: init at 1.0-77-gf646f65 Feb 29, 2024
@evils evils marked this pull request as draft February 29, 2024 08:13
@evils
Copy link
Member Author

evils commented Feb 29, 2024

converted to draft until the test can be sorted out
main issues there are:

  1. actually networking the two machines
  2. extracting the MAC, IPv4 and/or IPv6 addresses from the second machine, to check if they're detected on the first

@evils evils force-pushed the clarissa branch 2 times, most recently from 0011315 to 5d5f1c5 Compare February 29, 2024 10:07
@evils evils marked this pull request as ready for review February 29, 2024 10:07
@evils
Copy link
Member Author

evils commented Feb 29, 2024

actually waiting for the ping to succeed seems to be enough for the other machine's hostname to show up

ideally i'd get the addresses out of the other machine and check for those, but this will do as a basic test

@evils
Copy link
Member Author

evils commented Feb 29, 2024

i'm intentionally ignoring the failing nixpkgs-check-by-name check
this adds 3 very closely related packages all based on the same source
i'd like to keep them grouped in 1 directory, rather than add 3 by-name directories

@infinisil
Copy link
Member

@evils
Copy link
Member Author

evils commented Mar 6, 2024

@infinisil huh, thanks, i didn't think to look there because i didn't know by-name was now a requirement
(i don't see that in CONTRIBUTING.md)
(note that jumping to that anchor doesn't seem to work (because of the loading time of such a long page?))

the recommendation there does not provide enough context for me to be able to use it
(where should inherit be used?, what's meant with a local package set (local where?), foo_1 refers to ../tools, wouldn't this be ../to/tools?)
is there an example package that already does this?
(just grepping for inherit yields a bit too much noise to find such)

@evils evils changed the title Clarissa: init at 1.0-77-gf646f65 Clarissa: init at 1.0-80-g00c2581 Mar 8, 2024
Comment on lines +3624 to +3626
clarissa = callPackage ../tools/networking/clarissa { };
clar = callPackage ../tools/networking/clarissa/clar.nix { };
clar-oui = callPackage ../tools/networking/clarissa/oui.nix { };
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late reply, this will work:

Suggested change
clarissa = callPackage ../tools/networking/clarissa { };
clar = callPackage ../tools/networking/clarissa/clar.nix { };
clar-oui = callPackage ../tools/networking/clarissa/oui.nix { };
inherit ({
clarissa = callPackage ../tools/networking/clarissa { };
clar = callPackage ../tools/networking/clarissa/clar.nix { };
clar-oui = callPackage ../tools/networking/clarissa/oui.nix { };
}) clarissa clar clar-oui;

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.

None yet

3 participants