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

cryptsetup: revert last update #46151

Closed
wants to merge 2 commits into from
Closed

cryptsetup: revert last update #46151

wants to merge 2 commits into from
+12 −2

Conversation

oxij
Copy link
Member

@oxij oxij commented Sep 6, 2018

Motivation for this change

Fails tests.

Things done
  • Built.

Sorry, something went wrong.

oxij added 2 commits September 6, 2018 12:52
@oxij
Revert "cryptsetup: 2.0.3 -> 2.0.4"
ed77b69 
This reverts commit 0b701f4.

2.0.4 fails its own tests.
@oxij
cryptsetup: enable tests
1f5d05e 
(by default) so that updates like that won't happen again.
@oxij oxij changed the title revert last cryptsetup update cryptsetup: revert last update Sep 6, 2018
@oxij oxij mentioned this pull request Sep 6, 2018
Merged
4 tasks
@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Sep 6, 2018
@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: cryptsetup

Partial log (click to expand)

shrinking RPATHs of ELF executables and libraries in /nix/store/jg0w7p6vrvx3hjn4yn2gjs48fhxf94i2-cryptsetup-2.0.3-dev
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/jg0w7p6vrvx3hjn4yn2gjs48fhxf94i2-cryptsetup-2.0.3-dev/lib
patching script interpreter paths in /nix/store/jg0w7p6vrvx3hjn4yn2gjs48fhxf94i2-cryptsetup-2.0.3-dev
checking for references to /build in /nix/store/jg0w7p6vrvx3hjn4yn2gjs48fhxf94i2-cryptsetup-2.0.3-dev...
shrinking RPATHs of ELF executables and libraries in /nix/store/2crpk8cmzgciv7s9p5422ar1s4q5j862-cryptsetup-2.0.3-man
gzipping man pages under /nix/store/2crpk8cmzgciv7s9p5422ar1s4q5j862-cryptsetup-2.0.3-man/share/man/
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/2crpk8cmzgciv7s9p5422ar1s4q5j862-cryptsetup-2.0.3-man
checking for references to /build in /nix/store/2crpk8cmzgciv7s9p5422ar1s4q5j862-cryptsetup-2.0.3-man...

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: cryptsetup

Partial log (click to expand)

strip is /nix/store/y4ymnvgxygpq05h03kyzbj572zmh6zla-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/i88y2f56j4c7hizh6206v00lgvfsqj4g-cryptsetup-2.0.3-dev/lib
patching script interpreter paths in /nix/store/i88y2f56j4c7hizh6206v00lgvfsqj4g-cryptsetup-2.0.3-dev
checking for references to /build in /nix/store/i88y2f56j4c7hizh6206v00lgvfsqj4g-cryptsetup-2.0.3-dev...
shrinking RPATHs of ELF executables and libraries in /nix/store/xn69jmk6qxl1slhp8mxnr1y9cwkgdjw5-cryptsetup-2.0.3-man
gzipping man pages under /nix/store/xn69jmk6qxl1slhp8mxnr1y9cwkgdjw5-cryptsetup-2.0.3-man/share/man/
strip is /nix/store/y4ymnvgxygpq05h03kyzbj572zmh6zla-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/xn69jmk6qxl1slhp8mxnr1y9cwkgdjw5-cryptsetup-2.0.3-man
checking for references to /build in /nix/store/xn69jmk6qxl1slhp8mxnr1y9cwkgdjw5-cryptsetup-2.0.3-man...
/nix/store/3vv81qmzmnbg4rwb9vxx4x3nljya4x9m-cryptsetup-2.0.3

@Mic92 Mic92 added the 9.needs: port to stable A PR needs a backport to the stable release. label Sep 6, 2018
Mic92
Mic92 reviewed Sep 6, 2018
View reviewed changes
pkgs/os-specific/linux/cryptsetup/default.nix
@@ -38,6 +38,8 @@ stdenv.mkDerivation rec {
buildInputs = [ lvm2 json_c openssl libuuid popt ]
++ stdenv.lib.optional enablePython python2;

doCheck = true;
Copy link
Member

Choose a reason for hiding this comment

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

What was the failure message?

Copy link
Member

@Mic92 Mic92 Sep 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally we won't stuck on older versions forever or else we cannot get security updates.

Copy link
Member Author

Choose a reason for hiding this comment

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

It fails most "compat" tests with some strange "file not found" errors. It might be just a bug in the tests, but I would feel uneasy running cryptsetup that could break something that is "compat".

Copy link
Member

Choose a reason for hiding this comment

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

Is this with or without sandbox? The latest cryptsetup also checks block devices for other filesystems and prevent corruption that way. That's means I would also feel uneasy to not having this safety feature.

Copy link
Member

Choose a reason for hiding this comment

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

I cannot reproduce the failure. Tests works for me: https://gist.github.com/Mic92/a603aea57ecd079b76903a54d3e1dd90#file-cryptsetup-log-L406

Copy link
Member

@Mic92 Mic92 Sep 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

ok, File not found could be the script itself, please try to use patchShebangs instead:

https://gitlab.com/cryptsetup/cryptsetup/blob/master/tests/compat-test#L1

Copy link
Contributor

Choose a reason for hiding this comment

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

There seem to be 2 issues here:

  1. The file not found error above is sandbox-related (although patchShebangs is already used). Still trying to find the reason.

  2. With sandboxing disabled, the compat test runs and succeeds if the build directory is on an ext4 fs, but 2 test cases fail if the build directory is on a btrfs fs (which is my default here). This is weird. We cannot have test success depend on the what fs the build dir is on, so we will need to disable the failing cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, actually it's only one issue: the test uses an internal tool unit-utils-io to read/write blocks in the file blocwise_localfile. This file exists but unit-utils-io opens it with O_DIRECT https://gitlab.com/cryptsetup/cryptsetup/blob/master/tests/unit-utils-io.c#L92, which is forbidden on a tmpfs and on btrfs (which produces the file not found errors - tmpfs in sandbox, btrfs without sandbox) but allowed on ext4. This is just stupid.

@oxij @Mic92 what should we do? We could

  • revert to 2.0.3
  • just disable this filesystem-dependent test
  • create a nixos test to run complete the tests in a VM where we can control the file system type.

Copy link
Member

Choose a reason for hiding this comment

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

Either

  • disable this filesystem-dependent test
  • create a nixos test to run complete the tests in a VM where we can control the file system type.

O_DIRECT is not implemented on zfs. It is a braindead interface anyway.
Reverting just because a test does not work in our build environment is pointless as the test works perfectly fine without it.
Actually we already have integration tests for cryptsetup. And don't think we need to duplicate upstream testing beyond a certain level in our own systems. Most of the critical stuff in cryptsetup is implemented in the kernel anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm implementing a solution to patch out O_DIRECT and disable the remaining 4 test cases. PR will follow shortly.

@Mic92 Mic92 added this to the 18.09 milestone Sep 6, 2018
@Mic92 Mic92 removed the 9.needs: port to stable A PR needs a backport to the stable release. label Sep 6, 2018
@xeji xeji mentioned this pull request Sep 7, 2018
Merged
1 task
xeji added a commit to xeji/nixpkgs that referenced this pull request Sep 8, 2018
@xeji
cryptsetup: enable and fix tests
277db37 
Some tests use O_DIRECT which is filesystem dependent and fails in a
sandbox as well as on some filesystems without sandboxing.
Patch out O_DIRECT and disable the 4 test cases that still fail in a
sandbox. See discussion in NixOS#46151.
@xeji xeji closed this in #46346 Sep 8, 2018
xeji added a commit that referenced this pull request Sep 8, 2018
@xeji
cryptsetup: enable and fix tests (#46346)
0817f94 
Some tests use O_DIRECT which is filesystem dependent and fails in a
sandbox as well as on some filesystems without sandboxing.
Patch out O_DIRECT and disable the 4 test cases that still fail in a
sandbox. See discussion in #46151.

(cherry picked from commit 8c6cf3d)
xeji added a commit that referenced this pull request Sep 8, 2018
@xeji
cryptsetup: enable and fix tests (#46346)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
8c6cf3d 
Some tests use O_DIRECT which is filesystem dependent and fails in a
sandbox as well as on some filesystems without sandboxing.
Patch out O_DIRECT and disable the 4 test cases that still fail in a
sandbox. See discussion in #46151.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92

@xeji xeji

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100
Projects
None yet
Milestone
18.09
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@oxij @GrahamcOfBorg @Mic92 @xeji