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

httping: add libintl to buildInputs on Darwin #48188

Merged
merged 2 commits into from
Oct 14, 2018
Merged

httping: add libintl to buildInputs on Darwin #48188

merged 2 commits into from
Oct 14, 2018

Conversation

iwinux
Copy link
Contributor

@iwinux iwinux commented Oct 11, 2018

Motivation for this change
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 compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-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)

  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: darwin Running or building packages on Darwin 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Oct 11, 2018
@fpletz
Copy link
Member

fpletz commented Oct 11, 2018

@GrahamcOfBorg build httping

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: httping

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: httping

Partial log (click to expand)

these paths will be fetched (0.06 MiB download, 0.18 MiB unpacked):
  /nix/store/dhkqvk2nsp7ak6dgfg0mf83sk2b4j0kh-httping-2.5
copying path '/nix/store/dhkqvk2nsp7ak6dgfg0mf83sk2b4j0kh-httping-2.5' from 'https://cache.nixos.org'...
/nix/store/dhkqvk2nsp7ak6dgfg0mf83sk2b4j0kh-httping-2.5

@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Attempted: httping

Partial log (click to expand)

mkdir -p /nix/store/w41j06jr6cmk7fp59jqi4l6l4azcn1xd-httping-2.5//share/locale/nl/LC_MESSAGES
cp nl.mo /nix/store/w41j06jr6cmk7fp59jqi4l6l4azcn1xd-httping-2.5//share/locale/nl/LC_MESSAGES/httping.mo
mkdir -p /nix/store/w41j06jr6cmk7fp59jqi4l6l4azcn1xd-httping-2.5//share/locale/ru/LC_MESSAGES
cp ru.mo /nix/store/w41j06jr6cmk7fp59jqi4l6l4azcn1xd-httping-2.5//share/locale/ru/LC_MESSAGES/httping.mo
post-installation fixup
gzipping man pages under /nix/store/w41j06jr6cmk7fp59jqi4l6l4azcn1xd-httping-2.5/share/man/
strip is /nix/store/9xjkb4xz0b5lmizij9ppxy7lkxdxhx6b-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/w41j06jr6cmk7fp59jqi4l6l4azcn1xd-httping-2.5/bin
patching script interpreter paths in /nix/store/w41j06jr6cmk7fp59jqi4l6l4azcn1xd-httping-2.5
/nix/store/w41j06jr6cmk7fp59jqi4l6l4azcn1xd-httping-2.5

fpletz
fpletz reviewed Oct 11, 2018
View reviewed changes
pkgs/tools/networking/httping/default.nix Outdated
@@ -29,6 +29,6 @@ stdenv.mkDerivation rec {
'';
license = licenses.agpl3;
maintainers = with maintainers; [ rickynils ];
platforms = platforms.linux;
platforms = [ "x86_64-linux" ] ++ platforms.darwin;
Copy link
Member

Choose a reason for hiding this comment

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

Why restrict the package to x86_64-linux when it seems to work on all linux platforms, i.e. aarch64?

https://hydra.nixos.org/job/nixos/unstable-aarch64/nixpkgs.httping.aarch64-linux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fpletz That was a mistake. Now fixed :)

c0bw3b
c0bw3b reviewed Oct 11, 2018
View reviewed changes
Copy link
Contributor

@c0bw3b c0bw3b left a comment

Choose a reason for hiding this comment

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

Unrelated to your changes but there is also a problem with TLS transport since the server cert is only valid for vanheusden.com and not www.vanheusden.com

So src.url and meta.homepage also need updating:

url = "https://vanheusden.com/httping/${name}.tgz";
homepage = https://vanheusden.com/httping/

@iwinux
Copy link
Contributor Author

iwinux commented Oct 12, 2018

@c0bw3b Actually it has a GitHub repo: https://github.com/flok99/httping. Would it be better if we fetch source from GitHub?

@c0bw3b
Copy link
Contributor

c0bw3b commented Oct 12, 2018

For fetching the source archive yes. I guess the GH repo could be considered more stable over time.

Set the homepage to the "real" author page though.

matthewbauer
matthewbauer reviewed Oct 12, 2018
View reviewed changes
pkgs/tools/networking/httping/default.nix Outdated
@@ -10,7 +10,7 @@ stdenv.mkDerivation rec {
sha256 = "1y7sbgkhgadmd93x1zafqc4yp26ssiv16ni5bbi9vmvvdl55m29y";
};

buildInputs = [ fftw ncurses openssl ];
buildInputs = [ fftw ncurses openssl ] ++ (with stdenv; lib.optional isDarwin libintl);
Copy link
Member

Choose a reason for hiding this comment

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

Since libintl is null on Glibc Linux, you don't need the lib.optional part. It can just be listed a build inputs. As a plus, this makes your changes compatible with other libcs that don't come with libintl like Musl!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthewbauer I didn't know that. Thanks!

@iwinux
Copy link
Contributor Author

iwinux commented Oct 14, 2018

@c0bw3b I've updated src.url & meta.homepage. The checksum of tarball from GitHub is different from the original one, so I'm not sure whether it should be used or not.

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Oct 14, 2018
@c0bw3b c0bw3b self-assigned this Oct 14, 2018
c0bw3b
c0bw3b approved these changes Oct 14, 2018
View reviewed changes
Copy link
Contributor

@c0bw3b c0bw3b left a comment

Choose a reason for hiding this comment

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

nox-review ok / builds and runs

@c0bw3b
Copy link
Contributor

c0bw3b commented Oct 14, 2018

@iwinux this is good to go this way.

When pulling source archives from GitHub or other git forges the SHA hash is computed on the content of the archive after unpacking. That hash can be computed manually with nix-prefetch-url --unpack <url>.
And you also have to use fetchFromGitHub or fetchgit or fetchzip instead of fetchurl.

For this simple package we can avoid that and just fetch the stable source archive the author is publishing on its website.

@c0bw3b c0bw3b merged commit 9f8b40c into NixOS:master Oct 14, 2018
LnL7 pushed a commit that referenced this pull request Oct 29, 2018
@iwinux @LnL7
httping: add libintl to buildInputs on Darwin (#48188)
32c258d 
* httping: add libintl to buildInputs on Darwin

Otherwise it won't compile.

* httping: fix URLs

(cherry picked from commit 9f8b40c)
@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

@fpletz fpletz fpletz left review comments

@matthewbauer matthewbauer matthewbauer left review comments

@c0bw3b c0bw3b c0bw3b approved these changes

Assignees

@c0bw3b c0bw3b

Labels
6.topic: darwin Running or building packages on Darwin 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 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.
6 participants
@iwinux @fpletz @GrahamcOfBorg @c0bw3b @matthewbauer @Janik-Haag