thin-provisioning-tools: 0.7.5 -> 0.7.6, fix w/musl, enable parallel #40559
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Success on x86_64-linux (full log) Attempted: thin-provisioning-tools Partial log (click to expand)
|
|
(unsure who to ask to review, following github's suggestion! :)) |
Mic92
reviewed
May 15, 2018
| }; | ||
|
|
||
| nativeBuildInputs = [ autoreconfHook ]; | ||
|
|
||
| buildInputs = [ expat libaio boost ]; | ||
|
|
||
| postPatch = stdenv.lib.optional stdenv.hostPlatform.isMusl '' | ||
| sed -i -e '/PAGE_SIZE/d' -e '1i#include <limits.h>' \ | ||
| block-cache/io_engine.h unit-tests/io_engine_t.cc |
There was a problem hiding this comment.
Aye aye, sounds good! Just pushed commit that uses patch + comments explaining the changes.
|
Success on x86_64-linux (full log) Attempted: thin-provisioning-tools Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: thin-provisioning-tools Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: thin-provisioning-tools Partial log (click to expand)
|
|
No attempt on x86_64-darwin (full log) The following builds were skipped because they don't evaluate on x86_64-darwin: thin-provisioning-tools Partial log (click to expand)
|
|
No attempt on x86_64-darwin (full log) The following builds were skipped because they don't evaluate on x86_64-darwin: thin-provisioning-tools Partial log (click to expand)
|
|
Post-merge review welcome as always! :) |
Mic92
added a commit
to Mic92/thin-provisioning-tools
that referenced
this pull request
May 22, 2018
a) Fix build if limits.h provides definition for PAGE_SIZE, as musl does w/musl per XSI[1] although it's apparently optional [2]. This value is only provided when it's known to be a constant, to avoid the need to discover the value dynamically. b) If not using system-provided (kernel headers, or libc headers, or something) use the POSIX approach of querying the value dynamically using sysconf(_SC_PAGE_SIZE) instead of hardcoded value that hopefully is correct. [1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html [2] http://www.openwall.com/lists/musl/2015/09/11/8 This patch originate from: https://raw.githubusercontent.com/voidlinux/void-packages/a0ece13ad7ab2aae760e09e41e0459bd999a3695/srcpkgs/thin-provisioning-tools/patches/musl.patch and was also applied in NixOS: NixOS/nixpkgs#40559 cc @dtzWill
|
I felt free to send it upstream. |
jthornber
pushed a commit
to jthornber/thin-provisioning-tools
that referenced
this pull request
May 22, 2018
a) Fix build if limits.h provides definition for PAGE_SIZE, as musl does w/musl per XSI[1] although it's apparently optional [2]. This value is only provided when it's known to be a constant, to avoid the need to discover the value dynamically. b) If not using system-provided (kernel headers, or libc headers, or something) use the POSIX approach of querying the value dynamically using sysconf(_SC_PAGE_SIZE) instead of hardcoded value that hopefully is correct. [1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html [2] http://www.openwall.com/lists/musl/2015/09/11/8 This patch originate from: https://raw.githubusercontent.com/voidlinux/void-packages/a0ece13ad7ab2aae760e09e41e0459bd999a3695/srcpkgs/thin-provisioning-tools/patches/musl.patch and was also applied in NixOS: NixOS/nixpkgs#40559 cc @dtzWill
|
... and it was merged. |
|
@Mic92 aweseome, thank you very much!! (And thanks to upstream maintainers for accepting it! 😁) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix with musl, update and enable parallel building while visiting.
Recent musl touches up how/when
PAGE_SIZE/PAGESIZEare exported[1][2][3],but unfortunately that doesn't fix compilation here.
This fixes build by using PAGE_SIZE from limits.h explicitly,
instead of defining local version.
Unlikely to change but might as well use what libc exports
in case it has a different value.
[1] https://git.musl-libc.org/cgit/musl/commit/?id=8e1381be44642523b5cbd1bba4d7ca20ee920b85
[2] https://git.musl-libc.org/cgit/musl/commit/?id=6ecb9c14c429cc73ace937fd7459f58f0b7a8e6e
[3] https://git.musl-libc.org/cgit/musl/commit/?id=c9c2cd3e6955cb1d57b8be01d4b072bf44058762
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)