frescobaldi: 2.0.16 -> 3.0.0; fix build #38028
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
Ma27
force-pushed
the
cleanup-frescobaldi
branch
from
798016c
to
0aff815
Compare
Ma27
changed the title
|
as thsi fixes the broken |
|
Success on x86_64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
@GrahamcOfBorg build frescobaldi |
|
No attempt on x86_64-darwin (full log) The following builds were skipped because they don't evaluate on x86_64-darwin: frescobaldi Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
@grahamc is 3600s limit a known problem/limitation? /me was looking at |
|
It is a configuration parameter on builder machines, with 3600s being the recommended default.
|
jtojnar
reviewed
Mar 29, 2018
| ]; | ||
|
|
||
| postPatch = '' | ||
| substituteInPlace setup.py --replace "__POPPLER_INCLUDE_DIR__" "${poppler.dev}/include/poppler" |
There was a problem hiding this comment.
if you change it to @POPPLER_INCLUDE_DIR@, you can use substituteAll.
Ma27
force-pushed
the
cleanup-frescobaldi
branch
from
0aff815
to
4d1c71d
Compare
Ma27
commented
Mar 29, 2018
pkgs/misc/frescobaldi/default.nix
Outdated
|
|
||
| patches = [ ./setup.cfg.patch ./python-path.patch ]; |
There was a problem hiding this comment.
if these patches aren't used anymore, they should be removed as well...
Ma27
force-pushed
the
cleanup-frescobaldi
branch
from
4d1c71d
to
ddfe2d2
Compare
|
Failure on aarch64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
dotlambda
requested changes
Mar 29, 2018
pkgs/top-level/all-packages.nix
Outdated
| @@ -2366,7 +2366,7 @@ with pkgs; | |||
|
|
|||
| freetds = callPackage ../development/libraries/freetds { }; | |||
|
|
|||
| frescobaldi = callPackage ../misc/frescobaldi {}; | |||
| frescobaldi = python3Packages.callPackage ../misc/frescobaldi {}; | |||
There was a problem hiding this comment.
Please don't use pythonPackages.callPackage for applications so people can override python packages used by the application.
There was a problem hiding this comment.
Why would not it be possible? Is not that just newScope?
There was a problem hiding this comment.
@dotlambda I thought the same as @jtojnar, but I'm not entirely sure as I'm not a nixpkgs internals expert. I'll have a look at it in the next hours and comment here when I know more :)
There was a problem hiding this comment.
OK I just confirmed that overriding is possible in fact:
# override.nix
with import ./. {};
frescobaldi.override {
fetchFromGitHub = { ... }: "nothing-here-dude";
}When running nix-build override.nix on my local cleanup-frescobaldi branch I get this:
these derivations will be built:
/nix/store/dn1hpfw0f8lnak008wmmgdw9sd1jfal1-frescobaldi-3.0.0.drv
building '/nix/store/dn1hpfw0f8lnak008wmmgdw9sd1jfal1-frescobaldi-3.0.0.drv'...
unpacking sources
unpacking source archive nothing-here-dude
do not know how to unpack source archive nothing-here-dude
builder for '/nix/store/dn1hpfw0f8lnak008wmmgdw9sd1jfal1-frescobaldi-3.0.0.drv' failed with exit code 1
error: build of '/nix/store/dn1hpfw0f8lnak008wmmgdw9sd1jfal1-frescobaldi-3.0.0.drv' failed
This actually shows that I managed to override fetchFromGitHub and provoke a build failure in the override. So it seems that you can easily override its dependencies, right?
There was a problem hiding this comment.
It's definitely possible, but you'd have to override the entire propagatedBuildInputs. Let's say you want to use a beta version of pyqt5, then you can just do the following when not using pythonPackages.callPackage:
frescobaldi.override {
python = python.override {
packageOverrides = self: super: {
poppler-qt5 = super.poppler-qt5.overridePythonAttrs (oldAttrs: rec {
version = "1.0b1";
src = oldAttrs.src.override {
inherit version;
sha256 = "...";
};
});
};
};
}There was a problem hiding this comment.
That is not really unique to python, even if it is somewhat exaggerated by the need for package uniqueness. Perhaps there is a way to reScope a package, or apply overlay locally?
dotlambda
reviewed
Mar 30, 2018
| ]; | ||
|
|
||
| nativeBuildInputs = [ pkgconfig ]; | ||
| propagatedBuildInputs = [ sip qtbase.dev pyqt5.dev poppler ]; |
There was a problem hiding this comment.
Everything that's not a python package belongs into buildInputs.
pkgs/top-level/all-packages.nix
Outdated
| @@ -2366,7 +2366,7 @@ with pkgs; | |||
|
|
|||
| freetds = callPackage ../development/libraries/freetds { }; | |||
|
|
|||
| frescobaldi = callPackage ../misc/frescobaldi {}; | |||
| frescobaldi = python3Packages.callPackage ../misc/frescobaldi {}; | |||
There was a problem hiding this comment.
It's definitely possible, but you'd have to override the entire propagatedBuildInputs. Let's say you want to use a beta version of pyqt5, then you can just do the following when not using pythonPackages.callPackage:
frescobaldi.override {
python = python.override {
packageOverrides = self: super: {
poppler-qt5 = super.poppler-qt5.overridePythonAttrs (oldAttrs: rec {
version = "1.0b1";
src = oldAttrs.src.override {
inherit version;
sha256 = "...";
};
});
};
};
}
|
@FRidh should definitely have a look at this. |
|
@dotlambda I'm sorry if I'm misunderstanding your example, but what would be wrong with the following expression: I patched |
|
@Ma27 The issue happens when some dependency also depends on |
|
When applications require overriding libraries, then the overriding should be done inside the expression, not in |
|
@FRidh @jtojnar @dotlambda thanks a lot for elaborting. As two python maintainers are now in favor of injecting |
Ma27
force-pushed
the
cleanup-frescobaldi
branch
from
ddfe2d2
to
f613931
Compare
|
Success on x86_64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
@GrahamcOfBorg build python2.pkgs.poppler-qt5 python3.pkgs.poppler-qt5 python2.pkgs.python-ly python3.pkgs.python-ly |
|
Success on x86_64-linux (full log) Attempted: python2.pkgs.poppler-qt5, python3.pkgs.poppler-qt5, python2.pkgs.python-ly, python3.pkgs.python-ly Partial log (click to expand)
|
|
@Ma27 Would you mind making 3 commits for the 3 packages involved? |
3.0.0 works with Python 3 and QT5 (proivded by pyqt5). These fixes are another step towards NixOS#32883 by getting rid of the unused poppler-qt4. See https://hydra.nixos.org/build/71816154/log See ticket NixOS#36453
Ma27
force-pushed
the
cleanup-frescobaldi
branch
from
f613931
to
545495b
Compare
|
@dotlambda you're right, this makes the history more obvious :) |
|
Success on x86_64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: python2.pkgs.poppler-qt5, python3.pkgs.poppler-qt5, python2.pkgs.python-ly, python3.pkgs.python-ly Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: frescobaldi Partial log (click to expand)
|
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
3.0.0 works with Python 3 and QT5 (proivded by
pyqt5). These fixes areanother step towards #32883 by getting rid of the unused
poppler-qt4.See https://hydra.nixos.org/build/71816154/log
See ticket #36453
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)