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

zprint: init at 1.1.2 #123150

Merged
merged 2 commits into from May 24, 2021
Merged

zprint: init at 1.1.2 #123150

merged 2 commits into from May 24, 2021

Conversation

stelcodes
Copy link
Contributor

Motivation for this change

Adding zprint to nixpkgs, a clojure pretty printer and source code formatter.

Things done

Added myself to the maintainer list, added derivation for zprint

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

@stelcodes
Copy link
Contributor Author

Hey! This is my first attempt at not only NixOS packaging, but linux packaging in general. I based this derivation on the babashka derivation, since they are both built with graalvm-ce. One thing that was confusing was that babashka uses graalvm11-ce.meta.platforms which evaluates to [ "x86_64-linux" "x86_64-darwin" ] but on search.nixos.org babashka platforms appear as only "x86_64-linux". How does that work? Both packages can only build on x86_64-linux I believe because GraalVM requires a license to build a native-image on Darwin.

Comment on lines +8 to +11
src = fetchurl {
url =
"https://github.com/kkinnear/${pname}/releases/download/${version}/${pname}-filter-${version}";
sha256 = "1wh8jyj7alfa6h0cycfwffki83wqb5d5x0p7kvgdkhl7jx7isrwj";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

use fetchFromGitHub instead
And I think compiling the archive, based on the source code, is better than downloading it and then running some command on it. But someone else should confirm that

Comment on lines 34 to 35
mkdir -p $out/bin
cp ${pname} $out/bin/${pname}
Copy link
Contributor

Choose a reason for hiding this comment

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

can be replaced with the install command

@legendofmiracles
Copy link
Contributor

Result of nixpkgs-review pr 123150 run on x86_64-linux 1

1 package built:
  • zprint

with lib;
stdenv.mkDerivation rec {
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
with lib;
stdenv.mkDerivation rec {
stdenv.mkDerivation rec {

cp ${pname} $out/bin/${pname}
'';

meta = {
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
meta = {
meta = with lib; {

@lukegb
Copy link
Contributor

lukegb commented May 23, 2021

Thanks for the contribution! Do you mind making the changes above (splitting out the maintainer addition to a separate commit) as well as rebasing to remove the merge commit?

@lukegb lukegb changed the title Init zprint at 1.1.2 zprint: init at 1.1.2 May 24, 2021
@lukegb lukegb merged commit 9deeafd into NixOS:master May 24, 2021
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

5 participants