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

qscintilla: revert bump to fix the build #39860

Merged
merged 2 commits into from May 3, 2018
Merged

qscintilla: revert bump to fix the build #39860

merged 2 commits into from May 3, 2018

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented May 2, 2018 •
edited

Motivation for this change

This reverts the bump of qscintilla performed in #37330 as it actively breaks the qt5-darwin support added in 7343238.

Furthermore #37330 (namely 8ee6a03) never worked when building the qt4 variant nix-build -A qscintilla.

The original bump broke the following packages transitively:

  • pkgs.tora
  • pkgs.sqlitebrowser
  • pkgs.sonic-pi
  • and many more...

As I don't have the required resources to actively test/maintain the darwin compatibility of qscintilla and the qt4 variant never worked since the bump, I'd vote to revert the change for now and perform a bump with tested qt4 and darwin derivations.

/cc @ryantm @mpickering @peterhoeg

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: libsForQt5.qscintilla

Partial log (click to expand)

shrinking RPATHs of ELF executables and libraries in /nix/store/53d6x8465f976ci1l8hkgzspi12vzh2y-qscintilla-qt5-2.9.4
shrinking /nix/store/53d6x8465f976ci1l8hkgzspi12vzh2y-qscintilla-qt5-2.9.4/lib/libqscintilla2.so.12.0.2
strip is /nix/store/j75dgadrff2d1fyc4fczmcgqkid2imdx-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/53d6x8465f976ci1l8hkgzspi12vzh2y-qscintilla-qt5-2.9.4/lib
patching script interpreter paths in /nix/store/53d6x8465f976ci1l8hkgzspi12vzh2y-qscintilla-qt5-2.9.4
checking for references to /build in /nix/store/53d6x8465f976ci1l8hkgzspi12vzh2y-qscintilla-qt5-2.9.4...
postPatchMkspecs
fixQtBuiltinPaths: Fixing Qt builtin paths in `/nix/store/53d6x8465f976ci1l8hkgzspi12vzh2y-qscintilla-qt5-2.9.4/mkspecs/features/qscintilla2.prf'...
postPatchMkspecs
/nix/store/53d6x8465f976ci1l8hkgzspi12vzh2y-qscintilla-qt5-2.9.4

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: libsForQt5.qscintilla

Partial log (click to expand)

shrinking RPATHs of ELF executables and libraries in /nix/store/3zx47qf3xgnyzgzd3md842n6dhbf16ys-qscintilla-qt5-2.9.4
shrinking /nix/store/3zx47qf3xgnyzgzd3md842n6dhbf16ys-qscintilla-qt5-2.9.4/lib/libqscintilla2.so.12.0.2
strip is /nix/store/gp7fylxwn18b7pl2c18ks89hsiaxyfvf-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/3zx47qf3xgnyzgzd3md842n6dhbf16ys-qscintilla-qt5-2.9.4/lib
patching script interpreter paths in /nix/store/3zx47qf3xgnyzgzd3md842n6dhbf16ys-qscintilla-qt5-2.9.4
checking for references to /build in /nix/store/3zx47qf3xgnyzgzd3md842n6dhbf16ys-qscintilla-qt5-2.9.4...
postPatchMkspecs
fixQtBuiltinPaths: Fixing Qt builtin paths in `/nix/store/3zx47qf3xgnyzgzd3md842n6dhbf16ys-qscintilla-qt5-2.9.4/mkspecs/features/qscintilla2.prf'...
postPatchMkspecs
/nix/store/3zx47qf3xgnyzgzd3md842n6dhbf16ys-qscintilla-qt5-2.9.4

@ryantm ryantm mentioned this pull request May 2, 2018
Closed
@ryantm
Copy link
Member

ryantm commented May 2, 2018

@Ma27 Added issue to blacklist this for nixpkgs-update

@Ma27
Copy link
Member Author

Ma27 commented May 2, 2018

(short side note, I'm currently running nox-review to reduce the risk of breakage, but that may take some time)

Added issue to blacklist this for nixpkgs-update

This is definetely an appropriate measure to do for now I guess, however I think that the core issue behind the breakage we experience is a different one:

several packages in nixpkgs live in the "main" set (e.g. nixos.foo), but have specialized derivations in meta pages (e.g. nixos.metapackage.foo).

One example is qscintilla where the qt5 derivation lives in nixos.libsForQt5.qscintilla, but the qt4 derivation (which was obviously not tested) lives in nixos.qscintilla. Another specimen would be dlib with nixos.dlib and nixos.pythonPackages.dlib which use the same source. Unfortunately I don't know too much about how nixpkgs-update works ATM, but I guess that we'll have to enhance the testing logic a bit as this is definetely an avoidable issue :-)

@Ma27
Copy link
Member Author

Ma27 commented May 2, 2018 •
edited

nox-review seems fine:

Result in /tmp/nox-review-1iunr8nj
total 32
lrwxrwxrwx 1 ma27 users 52 May  2 20:08 result -> /nix/store/j9k3fxcplc7mh997l15dx6k4hwp45xdm-tora-3.1
lrwxrwxrwx 1 ma27 users 56 May  2 20:08 result-10 -> /nix/store/2rs6qyhmw8v315bqh18pk83pyb5zb3hk-octave-4.2.2
lrwxrwxrwx 1 ma27 users 60 May  2 20:08 result-11 -> /nix/store/nkq5ldzfdwzqn3nmj46m0xdj5kg9dyab-tortoisehg-4.5.2
lrwxrwxrwx 1 ma27 users 59 May  2 20:08 result-12 -> /nix/store/qd00krifrfpzkm3z6065c5h0w5shp9bk-sqliteman-1.2.0
lrwxrwxrwx 1 ma27 users 58 May  2 20:08 result-13 -> /nix/store/wk6221qsq1aqsw45d1npkm8prh4ki0p5-sonic-pi-3.0.1
lrwxrwxrwx 1 ma27 users 62 May  2 20:08 result-14 -> /nix/store/3p6rk7dw3sj5gdzclsp3jfyrxn2pqk34-openscad-2015.03-3
lrwxrwxrwx 1 ma27 users 64 May  2 20:08 result-2 -> /nix/store/zj3x270mdr203wgrs964z7arbwfvbwn5-octave-4.3.0pre23269
lrwxrwxrwx 1 ma27 users 56 May  2 20:08 result-3 -> /nix/store/0nl2gc98jl8cnpl659i61sa00c5mk2cw-octave-4.2.2
lrwxrwxrwx 1 ma27 users 64 May  2 20:08 result-4 -> /nix/store/bj07sqbx3vqy0scq8jrfh664q2qndmkn-qscintilla-qt4-2.9.4
lrwxrwxrwx 1 ma27 users 56 May  2 20:08 result-5 -> /nix/store/b93fqvyx6b7q2l8v2zz6mk9i3mzxyl9l-qgis-2.18.17
lrwxrwxrwx 1 ma27 users 70 May  2 20:08 result-6 -> /nix/store/hn5jzai45js2m5ypnaby6kwqqcancrrm-python2.7-qscintilla-2.9.4
lrwxrwxrwx 1 ma27 users 64 May  2 20:08 result-7 -> /nix/store/53d6x8465f976ci1l8hkgzspi12vzh2y-qscintilla-qt5-2.9.4
lrwxrwxrwx 1 ma27 users 64 May  2 20:08 result-8 -> /nix/store/a8i64jjjl7v7hrpzmpibqkvijzqcjz97-sqlitebrowser-3.10.1
lrwxrwxrwx 1 ma27 users 67 May  2 20:08 result-9 -> /nix/store/qsyxvyd6ms0xb5rfn7f4zp0gkvnwxwz7-minc-widgets-2016-04-20

@ryantm ryantm mentioned this pull request May 2, 2018
Open
@peterhoeg
Copy link
Member

Instead of reverting, I would much rather that we keep a separate qscintilla-qt4 around for 2.9 (several pyqt4 programs depend on qscintilla) and then keep the move to 2.10 around for qt5.

@lukeadams
Copy link
Contributor

lukeadams commented May 3, 2018 •
edited

@Ma27 There's a version bump (to 2.10.4) which fixes the qt4 variant It allows Octave to build on my workstation, so at least that works.

@Ma27
Copy link
Member Author

Ma27 commented May 3, 2018

@peterhoeg I'd actually agree with you, however libsForQt5.qscintilla never worked with version 2.10: https://hydra.nixos.org/job/nixos/trunk-combined/nixpkgs.libsForQt5.qscintilla.x86_64-linux

The main reason for this is that in 3391006 was added a darwin compliancy patch from the homebrew repository which fixes the XCode build of qscintilla.
x86_64-linux was broken as well as the patch was applied for each qt5 build (which I fixed in this PR as well).

As you can see on the nox-review output I posted above this will fix several packages on master. We should at least merge be153f6 to fix the Linux builds and maybe even the revert (and let somebody try another bump in case he's able to test (and optionally rebase) the Darwin patch).

@peterhoeg peterhoeg merged commit 773fe1f into NixOS:master May 3, 2018
@peterhoeg
Copy link
Member

Thanks for doing this - you are absolutely right.

@Ma27 Ma27 deleted the fix-qscintilla branch May 3, 2018 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Ma27 @GrahamcOfBorg @ryantm @peterhoeg @lukeadams