Improve sequoia package expression #65724
Merged
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
FRidh
requested changes
Aug 1, 2019
doronbehar
force-pushed
the
improve-sequoia
branch
from
86c89d7
to
811f852
Compare
jonringer
reviewed
Aug 1, 2019
jonringer
reviewed
Aug 1, 2019
There was a problem hiding this comment.
the python packages fail to build for me.
Fail log
Compiling rustc_version v0.2.3
error: failed to run custom build command for `openssl-sys v0.9.47`
Caused by:
process didn't exit successfully: `/build/source/target/debug/build/openssl-sys-a3307e1288dc484e/build-script-main` (exit code: 101)
--- stdout
cargo:rustc-cfg=const_fn
cargo:rerun-if-env-changed=X86_64_UNKNOWN_LINUX_GNU_OPENSSL_LIB_DIR
X86_64_UNKNOWN_LINUX_GNU_OPENSSL_LIB_DIR unset
cargo:rerun-if-env-changed=OPENSSL_LIB_DIR
OPENSSL_LIB_DIR unset
cargo:rerun-if-env-changed=X86_64_UNKNOWN_LINUX_GNU_OPENSSL_INCLUDE_DIR
X86_64_UNKNOWN_LINUX_GNU_OPENSSL_INCLUDE_DIR unset
cargo:rerun-if-env-changed=OPENSSL_INCLUDE_DIR
OPENSSL_INCLUDE_DIR unset
cargo:rerun-if-env-changed=X86_64_UNKNOWN_LINUX_GNU_OPENSSL_DIR
X86_64_UNKNOWN_LINUX_GNU_OPENSSL_DIR unset
cargo:rerun-if-env-changed=OPENSSL_DIR
OPENSSL_DIR unset
run pkg_config fail: "Failed to run `\"pkg-config\" \"--libs\" \"--cflags\" \"openssl\"`: No such file or directory (os error 2)"
--- stderr
thread 'main' panicked at '
Could not find directory of OpenSSL installation, and this `-sys` crate cannot
proceed without this knowledge. If OpenSSL is installed and this crate had
trouble finding it, you can set the `OPENSSL_DIR` environment variable for the
compilation process.
Make sure you also have the development packages of openssl installed.
For example, `libssl-dev` on Ubuntu or `openssl-devel` on Fedora.
If you're in a situation where you think the directory *should* be found
automatically, please open a bug at https://github.com/sfackler/rust-openssl
and include information about your system as well as this message.
$HOST = x86_64-unknown-linux-gnu
$TARGET = x86_64-unknown-linux-gnu
openssl-sys = 0.9.47
It looks like you're compiling on Linux and also targeting Linux. Currently this
requires the `pkg-config` utility to find OpenSSL but unfortunately `pkg-config`
could not be found. If you have OpenSSL installed you can likely fix this by
installing `pkg-config`.
', /build/sequoia-0.9.0-vendor/openssl-sys/build/find_normal.rs:150:5
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
warning: build failed, waiting for other jobs to finish...
error: build failed
make: *** [Makefile:48: build] Error 101
builder for '/nix/store/yd4348iiqvxmfms8cc29c4k2w1nzmr5d-sequoia-0.9.0.drv' failed with exit code 2
error: build of '/nix/store/yd4348iiqvxmfms8cc29c4k2w1nzmr5d-sequoia-0.9.0.drv' failed
|
I think if you null the phases this could work as expected. see the |
doronbehar
force-pushed
the
improve-sequoia
branch
from
811f852
to
dbbc94a
Compare
I've tested the build with As @FRidh has mentioned in his comment here I read the examples and now I think
Are you referring to @jonringer's comment on the BTW thanks for all the comments :) |
doronbehar
force-pushed
the
improve-sequoia
branch
from
dbbc94a
to
e7e24af
Compare
FRidh
reviewed
Aug 2, 2019
|
@doronbehar I meant how you have to manually override I believe that if you do that you'll just need to set Edit: rephrased since I might have been incoherent at the time of writing 😄 |
doronbehar
force-pushed
the
improve-sequoia
branch
from
e7e24af
to
188d993
Compare
|
All that's left is the comment that @worldofpeace made. I would also prefer if we would not have to explicitly invoke |
|
So are you suggesting to just source the basic |
|
@doronbehar I believe this should do what we need patchdiff --git a/pkgs/tools/security/sequoia/default.nix b/pkgs/tools/security/sequoia/default.nix
index 9f2a5ee4ddd..a71cd4aec2f 100644
--- a/pkgs/tools/security/sequoia/default.nix
+++ b/pkgs/tools/security/sequoia/default.nix
@@ -18,6 +18,8 @@ rustPlatform.buildRustPackage rec {
sha256 = "13dzwdzz33dy2lgnznsv8wqnw2501f2ggrkfwpqy5x6d1kgms8rj";
};
+ cargoSha256 = "1zcnkpzcar3a2fk2rn3i3nb70b59ds9fpfa44f15r3aaxajsdhdi";
+
nativeBuildInputs = [
pkgconfig
cargo
@@ -26,11 +28,13 @@ rustPlatform.buildRustPackage rec {
llvmPackages.libclang
llvmPackages.clang
];
+
checkInputs = lib.optionals pythonSupport [
pythonPackages.pytest
pythonPackages.pytestrunner
pythonPackages.setuptools
];
+
buildInputs = [
openssl
sqlite
@@ -38,10 +42,20 @@ rustPlatform.buildRustPackage rec {
capnproto
ensureNewerSourcesForZipFilesHook
]
- ++ lib.optionals pythonSupport [pythonPackages.python pythonPackages.cffi]
+ ++ lib.optionals pythonSupport [ pythonPackages.python pythonPackages.cffi ]
++ lib.optionals stdenv.isDarwin [ darwin.apple_sdk.frameworks.Security ]
;
+ makeFlags = [
+ "PREFIX=${placeholder ''out''}"
+ ];
+
+ installFlags = lib.optionals (!pythonSupport) [
+ "PYTHON=disable"
+ ] ++ lib.optionals pythonSupport [
+ "PYTHONPATH=${placeholder ''out''}/${pythonPackages.python.sitePackages}:$PYTHONPATH"
+ ];
+
LIBCLANG_PATH = "${llvmPackages.libclang}/lib";
preConfigure = ''
@@ -55,21 +69,11 @@ rustPlatform.buildRustPackage rec {
--replace '-O0 -g -Wall -Werror' '-g'
'';
- buildPhase = ''
- make PREFIX=$out build
- '';
- checkPhase = ''
- make check
- '';
-
- # Python bindings are enabled by default in the source distribution
- installPhase = if pythonSupport then ''
- make PREFIX=$out PYTHONPATH=$out/${pythonPackages.python.sitePackages}:$PYTHONPATH install
- '' else ''
- make PREFIX=$out PYTHON=disable install
- '';
-
- cargoSha256 = "1zcnkpzcar3a2fk2rn3i3nb70b59ds9fpfa44f15r3aaxajsdhdi";
+ # Don't use buildRustPackage phases, only use it for rust deps setup
+ configurePhase = null;
+ buildPhase = null;
+ checkPhase = null;
+ installPhase = null;
meta = with stdenv.lib; {
description = "A cool new OpenPGP implementation";
Though it did fail to build so it looks python related. |
jonringer
reviewed
Aug 5, 2019
|
not sure if i''m going crazy, but it seems to need to build the rust package several times... |
Do you have the build output? I did notice when building that it looked repetitive. |
|
Very long logs looks like the rust package is being built twice, then the wheel is built. Full nix-build -A sequoia logs |
|
With my patch @doronbehar? |
|
it might be worth while to have a |
|
I think the double build is because of the override in |
@johringer I've also noticed it seems to be building the package twice. According to upstream's Another option could be starting never the less with
In your mind, what files do you think should appear in the We can (I've had a discussion about it with @FRidh here) split the output to What do you think about the matter @worldofpeace?
I've pushed a new version which includes your suggestions. However, due to the concern stated above regarding "What |
doronbehar
force-pushed
the
improve-sequoia
branch
from
188d993
to
37f9ad3
Compare
|
the unwrapped package would have the build outputs the rust compilation. seeing as that takes a significant amount of time to recompile if we do an override. |
|
So the unwrapped package will essentially include everything that comes out of |
I think wrapped and unwrapped packages are confusing to users because they the paradigm isn't documented. However I'm not sure I can follow here what the issue was?
The |
|
@worldofpeace I gave a full test drive to the new version of the expression with the phases nullified and it doesn't seem to be working now, similarly to what you've experienced. I'll explain: First of all, it doesn't seem to be performing the tests anymore with the Secondly, not surprisingly, Where's the I'm really tempted towards reverting back your patch. |
Have you tried
Then don't use Note all of this information available in the
|
|
@worldofpeace it seems to be a good idea to export the correct variables in |
doronbehar
force-pushed
the
improve-sequoia
branch
from
37f9ad3
to
4becd79
Compare
|
@doronbehar When I build this and inspect It appears that you didn't use diffdiff --git a/pkgs/tools/security/sequoia/default.nix b/pkgs/tools/security/sequoia/default.nix
index c1ee16f996c..4673ca18c24 100644
--- a/pkgs/tools/security/sequoia/default.nix
+++ b/pkgs/tools/security/sequoia/default.nix
@@ -53,7 +53,7 @@ rustPlatform.buildRustPackage rec {
LIBCLANG_PATH = "${llvmPackages.libclang}/lib";
- preConfigure = ''
+ postPatch = ''
# otherwise, the check fails because we delete the `.git` in the unpack phase
substituteInPlace openpgp-ffi/Makefile \
--replace 'git grep' 'grep -R'
@@ -62,10 +62,12 @@ rustPlatform.buildRustPackage rec {
--replace '-O0 -g -Wall -Werror' '-g'
substituteInPlace ffi/examples/Makefile \
--replace '-O0 -g -Wall -Werror' '-g'
- '' + lib.optionalString pythonSupport ''
- installFlags="PYTHONPATH=$PYTHONPATH:$out/${pythonPackages.python.sitePackages}"
+ '';
+
+ preInstall = lib.optionalString pythonSupport ''
+ export installFlags="PYTHONPATH=$PYTHONPATH:$out/${pythonPackages.python.sitePackages}"
'' + lib.optionalString (!pythonSupport) ''
- installFlags="PYTHON=disable"
+ export installFlags="PYTHON=disable"
'';
# Don't use buildRustPackage phases, only use it for rust deps setup |
|
With that patch applied and without it, I don't see any Currently, |
I've already responded to that in #65724 (comment) The reason the checks weren't run is because the default Updated patch that I testeddiff --git a/pkgs/tools/security/sequoia/default.nix b/pkgs/tools/security/sequoia/default.nix
index c1ee16f996c..5d9ffca6937 100644
--- a/pkgs/tools/security/sequoia/default.nix
+++ b/pkgs/tools/security/sequoia/default.nix
@@ -51,9 +51,13 @@ rustPlatform.buildRustPackage rec {
"PREFIX=${placeholder ''out''}"
];
+ buildFlags = [
+ "build-release"
+ ];
+
LIBCLANG_PATH = "${llvmPackages.libclang}/lib";
- preConfigure = ''
+ postPatch = ''
# otherwise, the check fails because we delete the `.git` in the unpack phase
substituteInPlace openpgp-ffi/Makefile \
--replace 'git grep' 'grep -R'
@@ -62,17 +66,17 @@ rustPlatform.buildRustPackage rec {
--replace '-O0 -g -Wall -Werror' '-g'
substituteInPlace ffi/examples/Makefile \
--replace '-O0 -g -Wall -Werror' '-g'
- '' + lib.optionalString pythonSupport ''
- installFlags="PYTHONPATH=$PYTHONPATH:$out/${pythonPackages.python.sitePackages}"
+ '';
+
+ preInstall = lib.optionalString pythonSupport ''
+ export installFlags="PYTHONPATH=$PYTHONPATH:$out/${pythonPackages.python.sitePackages}"
'' + lib.optionalString (!pythonSupport) ''
- installFlags="PYTHON=disable"
+ export installFlags="PYTHON=disable"
'';
# Don't use buildRustPackage phases, only use it for rust deps setup
configurePhase = null;
- buildPhase = ''
- make $makeFlags build-release
- '';
+ buildPhase = null;
doCheck = true;
checkPhase = null;
installPhase = null;
|
doronbehar
force-pushed
the
improve-sequoia
branch
from
4becd79
to
ee0a29d
Compare
|
Thanks for the thorough clarification and for the patch. I've almost figured it out after reading the docs and the source code of With that patch applied, and now pushed, and since you've tested that yourself (BTW thanks for investing once more computing power into this) can we please merge now? EDIT: What say you @worldofpeace? |
I think you might have seen my comment mid composition. I'm thinking there's only one more issue, this needs to be a split package/multiple output derivation. Please see the nixpkgs manual on this subject and I'd recommend trying to figure out the appropriate outputs by examining the already built output of the current expression. |
|
Multiple outputs may be a nice extra, but should not be immediately required. |
Indeed, feel free to merge this as you see fit. |
|
I tried to use the following outputs: And it failed with the following log at the fixup phase: I agree we should try to split the outputs. But even using only the outputs I couldn't figure out yet exactly where's the failure.. Would you be willing to merge this only if the outputs are split? Would it be OK if I'd totally drop the use of standard |
Would you like something more like: |
jonringer
reviewed
Aug 13, 2019
|
|
||
| # Don't use buildRustPackage phases, only use it for rust deps setup | ||
| configurePhase = null; | ||
| buildPhase = null; |
There was a problem hiding this comment.
you will probably want to use the doXXX options instead
Suggested change
| buildPhase = null; | |
| doBuild = false; |
| "PREFIX=${placeholder ''out''}" | ||
| ]; | ||
|
|
||
| buildFlags = [ |
There was a problem hiding this comment.
you're skipping the build phase now, does this even get used?
| ''; | ||
|
|
||
| preInstall = lib.optionalString pythonSupport '' | ||
| export installFlags="PYTHONPATH=$PYTHONPATH:$out/${pythonPackages.python.sitePackages}" |
There was a problem hiding this comment.
prevent override other flags
Suggested change
| export installFlags="PYTHONPATH=$PYTHONPATH:$out/${pythonPackages.python.sitePackages}" | |
| export installFlags="PYTHONPATH=$PYTHONPATH:$out/${pythonPackages.python.sitePackages} ${installFlags}" |
|
you may want to try: also, the build will fail if nothing gets put on a given path, which is why bin fails, there's not executable being moved there, so it's empty, and fails. if the build is having trouble placing something, then i would suggest doing: The easier option would be to remove the bin output altogether, although less "correct", it would probably fix a lot of your issues. The benefits will probably be marginal at that point though, because the executables in out will most likely need the lib closure (which is typically the largest). The dev closure will probably just have headers, pkgconfig info, a may a few other things, so it will be significantly smaller, but not significant in practice. |
|
Thanks for the suggestions @jonringer. I'll address the points you've made in the review there. As for the Also, the executables don't depend on the While reading Does this line: nixpkgs/pkgs/build-support/setup-hooks/multiple-outputs.sh Lines 36 to 39 in 6b135bf mean that it's just always moved to |
|
I don't think we should change anything else. I can merge this manually. |
worldofpeace
force-pushed
the
improve-sequoia
branch
from
ee0a29d
to
db661a1
Compare
- Add the package to the pythonPackages' attribute set.
- Make the python support overrideable
We use the pythonSupport argument.
- Rename sequoia-tool -> sequoia
We provide the whole ecosystem which includes:
* ffi bindings to Python and C
* zsh and bash completion for `sq` and `sqv` executables.
- Meta:
* Use a string as the homepage URL (plain URLs are deprecated).
* Change description of package to fit upstream and the files we
actually install.
* Add @doronbehar as maintainer.
worldofpeace
force-pushed
the
improve-sequoia
branch
from
db661a1
to
701c788
Compare
|
Thanks @worldofpeace ! Finally... 😌. |
10 tasks
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.
Motivation for this change
As discussed at #65475, these are the changes I performed on the existing sequoia package:
make checkandmake installascheckPhaseandinstallPhase, respectively.pythonPackages' attribute set.pythonSupportargument.sqandsqvexecutables.make checkandmake installascheckPhaseandinstallPhase, respectively.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @minijackson .