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

Issue 61250 coreutils disable test on musl #61471

Merged
merged 2 commits into from Aug 14, 2019

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented May 13, 2019

Motivation for this change

Fixes #61250 (comment)

CC @matthewbauer @tobim

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@tobim
Copy link
Contributor

tobim commented May 16, 2019

Nice work tracking that down.
I wonder if coreutils should have tzdata included in buildInputs, so that these are actually executed in sandboxed mode?

@dtzWill
Copy link
Member

dtzWill commented May 16, 2019

Awesome investigation! Minor nit: if only needed for testing, should be added to checkInputs. Although I don't think tzdata will help although I could be wrong.

As for this PR, I would prefer using upstream patch if possible... is it correct that the best patch available is the one from the mailing list? ( https://lists.gnu.org/archive/html/coreutils/2019-05/txtrygo35HvcI.txt or so )

@nh2
Copy link
Contributor Author

nh2 commented May 16, 2019

As for this PR, I would prefer using upstream patch if possible... is it correct that the best patch available is the one from the mailing list?

@dtzWill Right now there's no upstream patch; I understood the posted one as "here's a patch, can you try it", and it looks reasonable to me, but I wouldn't call it upstream just yet.

I just gave a super quick shot with

         ++ optional stdenv.hostPlatform.isMusl (fetchpatch {
            url = https://lists.gnu.org/archive/html/coreutils/2019-05/txtrygo35HvcI.txt;
            name = "0001-tests-avoid-false-positive-in-date-debug-test.patch";
            sha256 = "1zg9m79x1i2nifj4kb0waf9x3i5h6ydkypkjnbsb9rnwis8rqypa"; # made up
         })

but somehow fetchpatch doesn't work in this file, I get

error: anonymous function at /home/niklas/src/haskell/static-haskell-nix/nixpkgs/pkgs/build-support/fetchurl/boot.nix:5:1 called with unexpected argument 'meta', at /home/niklas/src/haskell/static-haskell-nix/nixpkgs/pkgs/build-support/fetchpatch/default.nix:14:1

fetchurl works as normal.

Do you want me to use

         ++ optional stdenv.hostPlatform.isMusl (fetchurl {
            url = https://lists.gnu.org/archive/html/coreutils/2019-05/txtrygo35HvcI.txt;
            sha256 = "0blllqds5vkk16nahnakh8krqkk34x6i8hzlvkxbvlkkm79r5214";
         })

?

I wonder if coreutils should have tzdata included in buildInputs, so that these are actually executed in sandboxed mode?

@tobim Sounds like a reasonable idea, more tests being run sounds good. checkInputs as @dtzWill says seems like the right place for it. You need to check though if that helps as the functionality may look in /etc only, I'm not sure. In any case, that's for a different PR, do you want to take that item @tobim? As I'm quite busy with the swathe of small musl related fixes.

@dtzWill
Copy link
Member

dtzWill commented May 16, 2019 via email

nh2 added a commit to nh2/nixpkgs that referenced this pull request May 16, 2019
@nh2
Copy link
Contributor Author

nh2 commented May 16, 2019

@dtzWill Sounds good, should we merge then?

So that more patches can easily be added and commented.
@nh2 nh2 force-pushed the issue-61250-coreutils-disable-test-on-musl branch from 3bf3f18 to 29dd25b Compare August 10, 2019 04:35
@nh2
Copy link
Contributor Author

nh2 commented Aug 10, 2019

I've switched to using the upstream patch from coreutils/coreutils@0251229.

Ready to merge from my side, if it builds fine.

@nh2
Copy link
Contributor Author

nh2 commented Aug 13, 2019

@GrahamcOfBorg test pkgsMusl.coreutils

@nh2
Copy link
Contributor Author

nh2 commented Aug 13, 2019

@GrahamcOfBorg build pkgsMusl.coreutils

@nh2
Copy link
Contributor Author

nh2 commented Aug 13, 2019

@GrahamcOfBorg build pkgsMusl.coreutils

(I got building of '/...-gcc-7.4.0.drv' timed out after 3600 seconds, and @samueldr explained the timeout is for the whole build)

@tobim
Copy link
Contributor

tobim commented Aug 13, 2019

LGTM

@nh2 nh2 merged commit 19b043f into NixOS:master Aug 14, 2019
chkno added a commit to chkno/nixpkgs that referenced this pull request Jan 30, 2020
cgit cannot serve patches with stable hashes, so store these patches
in-tree.  cgit community discussion about this problem:
https://lists.zx2c4.com/pipermail/cgit/2017-February/003470.html

We pull the patches in-tree rather than strip cgit footers with fetchpatch
because per NixOS#61471 (comment)
dependencies of fetchpatch cannot use fetchpatch.

Verification that the only difference between the live page, the
patch committed here, and the version cached under the old hash at
tarballs.nixos.org is the cgit version footer:

$ curl -s -L http://tarballs.nixos.org/sha256/"$(nix-hash --type sha256 --to-base16 0iw0lk0yhnhvfjzal48ij6zdr92mgb84jq7fwryy1hdhi47hhq64)" > Allow_input_files_to_be_missing_for_ed-style_patches.patch
$ diff -U0 --label cgit-live <( curl -s -L https://git.savannah.gnu.org/cgit/patch.git/patch/?id=b5a91a01e5d0897facdd0f49d64b76b0f02b43e1 ) Allow_input_files_to_be_missing_for_ed-style_patches.patch
--- cgit-live
+++ Allow_input_files_to_be_missing_for_ed-style_patches.patch  2020-01-29 17:22:00.077312937 -0800
@@ -32 +32 @@
-cgit v1.2.1
+cgit v1.0-41-gc330

$ curl -s -L http://tarballs.nixos.org/sha256/"$(nix-hash --type sha256 --to-base16 1bpy16n3hm5nv9xkrn6c4wglzsdzj3ss1biq16w9kfv48p4hx2vg)" > CVE-2018-1000156.patch
$ diff -U0 --label cgit-live <( curl -s -L https://git.savannah.gnu.org/cgit/patch.git/patch/?id=123eaff0d5d1aebe128295959435b9ca5909c26d ) CVE-2018-1000156.patch
--- cgit-live
+++ CVE-2018-1000156.patch      2020-01-29 17:23:41.021116969 -0800
@@ -210 +210 @@
-cgit v1.2.1
+cgit v1.0-41-gc330
chkno added a commit to chkno/nixpkgs that referenced this pull request Jan 30, 2020
cgit cannot serve patches with stable hashes, so store these patches
in-tree.  cgit community discussion about this problem:
https://lists.zx2c4.com/pipermail/cgit/2017-February/003470.html

We pull the patches in-tree rather than strip cgit footers with fetchpatch
because per NixOS#61471 (comment)
dependencies of fetchpatch cannot use fetchpatch.

Verification that the only difference between the live page, the
patch committed here, and the version cached under the old hash at
tarballs.nixos.org is the cgit version footer:

$ curl -s -L http://tarballs.nixos.org/sha256/"$(nix-hash --type sha256 --to-base16 0iw0lk0yhnhvfjzal48ij6zdr92mgb84jq7fwryy1hdhi47hhq64)" > Allow_input_files_to_be_missing_for_ed-style_patches.patch
$ diff -U0 --label cgit-live <( curl -s -L https://git.savannah.gnu.org/cgit/patch.git/patch/?id=b5a91a01e5d0897facdd0f49d64b76b0f02b43e1 ) Allow_input_files_to_be_missing_for_ed-style_patches.patch
--- cgit-live
+++ Allow_input_files_to_be_missing_for_ed-style_patches.patch  2020-01-29 17:22:00.077312937 -0800
@@ -32 +32 @@
-cgit v1.2.1
+cgit v1.0-41-gc330

$ curl -s -L http://tarballs.nixos.org/sha256/"$(nix-hash --type sha256 --to-base16 1bpy16n3hm5nv9xkrn6c4wglzsdzj3ss1biq16w9kfv48p4hx2vg)" > CVE-2018-1000156.patch
$ diff -U0 --label cgit-live <( curl -s -L https://git.savannah.gnu.org/cgit/patch.git/patch/?id=123eaff0d5d1aebe128295959435b9ca5909c26d ) CVE-2018-1000156.patch
--- cgit-live
+++ CVE-2018-1000156.patch      2020-01-29 17:23:41.021116969 -0800
@@ -210 +210 @@
-cgit v1.2.1
+cgit v1.0-41-gc330
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