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

lipx: init at 1.2 #65055

Closed
wants to merge 1 commit into from
Closed

lipx: init at 1.2 #65055

wants to merge 1 commit into from

Conversation

coreyoconnor
Copy link
Contributor

Motivation for this change

add IPS patching tool

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

with lib;

stdenv.mkDerivation rec {
name = "lipx-${version}";
Copy link
Member

Choose a reason for hiding this comment

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

Please use pname.

src = fetchFromGitHub {
owner = "kylon";
repo = "Lipx";
rev = "1c01fcff06f1015750ad5461d5c898cba692326d";
Copy link
Member

Choose a reason for hiding this comment

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

Please either move the version to unstable-${date} or use the commit rev of the "release", which in this case looks like e66dc12c6c22c060e5c24fdf2698d7e6c2543b7b for 1.2.?

patchShebangs $out/bin/lipx
'';

meta = {
Copy link
Member

Choose a reason for hiding this comment

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

License is missing, can you please find out?

@worldofpeace
Copy link
Contributor

I don't think just a single python script like this is a good fit for nixpkgs.
NUR would be a better place or in your personal overlay.

@coreyoconnor
Copy link
Contributor Author

The number of files and language is not a metric for determining inclusion.

In this case, the program is a useful addition to the already existing emulation tools. Despite the rather odd source. However, I won't be furthering this pull request so ah well! I'll keep it in my fork; easier than NUR ;)

@worldofpeace
Copy link
Contributor

The number of files and language is not a metric for determining inclusion.

The actual metric I use for determining inclusion is how many users there could be and if it will be maintained in nixpkgs and upstream. That's not to mean it couldn't be useful to someone, I don't think the goal is to be a packages collection of every piece of software ever written.

@coreyoconnor
Copy link
Contributor Author

That is a fine metric. Stating that originally would of been reasonable.

@worldofpeace
Copy link
Contributor

That is a fine metric. Stating that originally would of been reasonable.

Thanks. I'll keep that in mind if I close someone's PR based on that metric so it's clear.
I can see how that was confusing outright.

@coreyoconnor
Copy link
Contributor Author

Thinking on this further... Not including low-impact packages from nixpkgs should be applied more. That leads to the question: How should "low impact" be determined? Lots of options and IMO nothing clear yet. In the meantime, I propose adding a label to PRs that have this feature. Enable quantifying to a degree.

(While I think there are usability issues with using NUR/overlays instead those are separate issues)

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/we-need-more-defined-guidelines-for-package-inclusion/3592/2

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

4 participants