Skip to content

catcli: init at 0.5.13 #84078

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
Merged
merged 2 commits into from
Apr 11, 2020
Merged

catcli: init at 0.5.13 #84078

merged 2 commits into from
Apr 11, 2020
+38 −0

Conversation

petersjt014
Copy link
Member

@petersjt014 petersjt014 commented Apr 2, 2020 •
edited
Loading

Motivation for this change

It's a neat tool.

Things done

Added the package, included tests found and done automatically by the buildPythonApplication function. Works with both x86-64 and i686. Aarch64 does not build with pkgsCross for reasons that I'm not sure of yet, but it does build natively.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@petersjt014 petersjt014 changed the title added catcli package, added petersjt014 (me) as maintainer catcli: init at 0.5.13 Apr 2, 2020
@petersjt014 petersjt014 mentioned this pull request Apr 2, 2020
Closed
@petersjt014 petersjt014 closed this Apr 2, 2020
@petersjt014 petersjt014 reopened this Apr 2, 2020
@petersjt014
Copy link
Member Author

Had to undo/redo the commit to split adding the package and myself as a maintainer into separate commits.

evils
evils suggested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@evils evils left a comment

Choose a reason for hiding this comment

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

none of this seems relevant to the grahamcofborg failure...

pkgs/tools/filesystems/catcli/default.nix Outdated
homepage = "https://github.com/deadc0de6/catcli";
license = licenses.gpl3;
maintainers = with maintainers; [ petersjt014 ];
platforms = with platforms; [ x86_64-linux i686-linux ];
Copy link
Member

Choose a reason for hiding this comment

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

it builds on my pi...

Suggested change
platforms = with platforms; [ x86_64-linux i686-linux ];
platforms = platforms.all;

Copy link
Member Author

@petersjt014 petersjt014 Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just tried and it builds natively on mine too.
Unfortuantely, the raspberryPi cross-build target still doesn't work (and I haven't tested it on macos, any of the bsds or anything else) so I'm hesitant to use all for now.

If there's a platform option that just covers x64/i686/aarch64 I would feel comfortable switching to that one though. There isn't one that I can find. I looked around and saw that a number of packages (such as yi) do just list those three, so I think it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

For anyone wondering this is the build log for raspberryPi: http://0x0.st/iu-6.txt

pkgs/tools/filesystems/catcli/default.nix

propagatedBuildInputs = [ docopt anytree ];

postPatch = '' patchShebangs . '';
Copy link
Member

Choose a reason for hiding this comment

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

what necessitated this?
it seems to work without it (though i am not familiar with this tool)

Copy link
Member Author

Choose a reason for hiding this comment

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

here's where I was pointed to when I asked. Apparently it's recommended to catch subtle path-related bugs (which could come up later if new deps are introduced). I don't think it's doing much now but it's also not harming anything.

Copy link
Member

Choose a reason for hiding this comment

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

the fixupPhase already does this
going by the fact that without this line the wrapped script has a shebang to python in the nix store, i'm guessing it also runs for buildPythonApplication

@petersjt014 petersjt014 requested a review from evils April 3, 2020 04:40
@petersjt014
Copy link
Member Author

petersjt014 commented Apr 3, 2020 •
edited
Loading

The commit history's a tad ugly, but (fixed) the package/maintainer files are all tidily formatted and it builds on all three listed platforms as advertised. It should be ready to merge.

evils
evils reviewed Apr 3, 2020
View reviewed changes
Copy link
Member

@evils evils left a comment

Choose a reason for hiding this comment

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

besides the postPatch line bothering me for no good reason, this looks good to me
i'd love to know what grahamcofbork is on about...

cole-h
cole-h requested changes Apr 4, 2020
View reviewed changes
pkgs/tools/filesystems/catcli/default.nix Outdated
buildPythonApplication rec {

pname = "catcli";
version = "v0.5.13";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version = "v0.5.13";
version = "0.5.13";

Versions should not include letters.

pkgs/tools/filesystems/catcli/default.nix Outdated
src = fetchFromGitHub {
owner = "deadc0de6";
repo = pname;
rev = version;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rev = version;
rev = "v${version}";

As a result of the above.

pkgs/tools/filesystems/catcli/default.nix Outdated
homepage = "https://github.com/deadc0de6/catcli";
license = licenses.gpl3;
maintainers = with maintainers; [ petersjt014 ];
platforms = with platforms; [ x86_64-linux aarch64-linux i686-linux ];
Copy link
Member

Choose a reason for hiding this comment

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

Uusually it's fine to just leave this as:

Suggested change
platforms = with platforms; [ x86_64-linux aarch64-linux i686-linux ];
platforms = platforms.linux;

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Apr 4, 2020
Ma27
Ma27 requested changes Apr 10, 2020
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Please squash the last two commits together. Otherwise LGTM, just tested locally.

@petersjt014 petersjt014 requested a review from Ma27 April 11, 2020 07:27
@Ma27 Ma27 merged commit 529465d into NixOS:master Apr 11, 2020
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evils evils

@cole-h cole-h

@Ma27 Ma27

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 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. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@petersjt014 @Ma27 @cole-h @evils @Janik-Haag