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.

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

qt5-packages.nix: consistently make all libraries with same qt5 version #102168

Merged
merged 4 commits into from
Jan 10, 2021

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Oct 30, 2020

Motivation for this change

Reduce the chance of mixing Qt versions by having Qt-based libraries in a separate package set.

Builds upon #101369.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

To do

@FRidh FRidh changed the title Qt515 qt5-packages.nix: consistently make all libraries with same qt5 version Oct 30, 2020
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: qt/kde 8.has: module (update) This PR changes an existing module in `nixos/` 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 30, 2020
@SuperSandro2000

This comment has been minimized.

@FRidh FRidh force-pushed the qt515 branch 2 times, most recently from be86aed to c1d711c Compare November 1, 2020 10:17
@ofborg ofborg bot removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 1, 2020
pkgs/top-level/qt-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@doronbehar
Copy link
Contributor

Long term thinking: How about changing a bit the attributes layout to look like this (json5):

qt5 = {
  // Same as today
  qtbase = ..
  // Other qt modules
  libraries = {
    // All what was previously in libsForQt5
  },
  apps = {
    // the apps from qt5-applications.nix
  }
}

And so on, with qt515 and qt514 etc.

@ttuegel
Copy link
Member

ttuegel commented Dec 5, 2020

How about changing a bit the attributes layout to look like this (json5):

qt5 = {
  // Same as today
  qtbase = ..
  // Other qt modules
  libraries = {
    // All what was previously in libsForQt5
  },
  apps = {
    // the apps from qt5-applications.nix
  }
}

And so on, with qt515 and qt514 etc.

I don't really like this beacuse, as you rightly pointed out, the distinction between "library" and "application" is flexible.

I suggest the following, which I think is the same as the situation for Python right now:

  1. All packages that depend on Qt go into qt-packages.nix. Basically, taking these changes to the next level.
  2. Packages in qt-packages.nix get their dependencies from that set first, so that they see a consistent set of dependencies.
  3. In all-packages.nix, we have attributes for the entire qt-packages.nix set instantiated with different versions of Qt: qtPackages5_14, qtPackages5_15, etc. The default version will be qtPackages5 = qtPackages5_yy. Hydra will not recurse into these sets.
  4. For stand-alone packages (that is: applications) add an attribute to all-packages.nix like konsole = qtPackages5.konsole. If the package needs to be pinned to a certain version of Qt, the maintainer can do that, too.

I think this would fix the mixed-version problems we have. The one problem: I don't know how to make this work with PyQt packages; they would somehow need to live in two package sets at once. I guess we can leave PyQt in pythonPackages (built with all versions of Python) and pin it to a particular version of Qt because all Python packages that depend on Qt should get it through PyQt so there is no risk of mixing versions.

@FRidh
Copy link
Member Author

FRidh commented Dec 11, 2020

For clarity I've kept libsForQt5 for now. Once everything else is fine, we can do a simple replacement to the chosen name.

If what's here now is fine, I'll move over more callPackage lines into qt-packages.nix.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Dec 11, 2020
@FRidh FRidh requested a review from ttuegel December 11, 2020 19:10
@FRidh FRidh added this to the 21.03 milestone Dec 11, 2020
@FRidh FRidh changed the base branch from master to staging January 10, 2021 15:00
@FRidh FRidh merged commit 38472ca into NixOS:staging Jan 10, 2021
@FRidh FRidh deleted the qt515 branch January 10, 2021 15:01
@FRidh
Copy link
Member Author

FRidh commented Jan 10, 2021

If what's here now is fine, I'll move over more callPackage lines into qt-packages.nix.

Did not do this eventually and this is now a follow-up task. Resolved the merge conflicts and merged this to avoid getting more conflicts.

@FRidh FRidh mentioned this pull request Jan 11, 2021
@SuperSandro2000
Copy link
Member

This PR broke eval on staging because fcitx-qt5 is no longer available. 2e9c639#diff-ab5748dc9567516fefba8344056b51ec1866adeace380f46e58a7af3d619ea22L15904

@cole-h
Copy link
Member

cole-h commented Jan 11, 2021

Rather, fcitx5-qt; #104658 and https://gist.github.com/8b2da452fad700c6e82220ae8039cca5.

EDIT: #108950 is where this cropped up.

FRidh added a commit that referenced this pull request Jan 11, 2021
#102168 moved Qt imports to qt5-packages.nix
and #104658 added a new package on the old
location.
@FRidh
Copy link
Member Author

FRidh commented Jan 11, 2021

Thanks. Fixed with 05ca995.

@anund
Copy link
Contributor

anund commented Jan 23, 2021

Is there an additional change to turn error: undefined variable 'kdeApplications' at ... into an informative message? Looks like the equivalent for kdeApplications.dolphin-plugins is now plasma5Packages.dolphin-plugins.

@anund
Copy link
Contributor

anund commented Jan 25, 2021

or plasma5Packages.kdeApplications.dolphin-plugins.

@jorsn
Copy link
Member

jorsn commented Feb 9, 2021

The one problem: I don't know how to make this work with PyQt packages; they would somehow need to live in two package sets at once. I guess we can leave PyQt in pythonPackages (built with all versions of Python) and pin it to a particular version of Qt because all Python packages that depend on Qt should get it through PyQt so there is no risk of mixing versions.
-- @ttuegel (#102168 (comment))

If you have scope A with subscopes B1, B2 and C and you want to change the version of B1 to B2 for use in C, you have to override B1 in A i.e. set A.C = (A.extend (self: super: { B1 = A.B2; })).C to avoid inconsistent versions at some level. So PyQt5 needs to be pinned to the same qt5 that the whole python closure uses. If python used another qt version than the tree-wide default this would probably lead to the same practically unmanageable mess we had in September (see #99956).

@jorsn jorsn mentioned this pull request Feb 9, 2021
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: qt/kde 8.has: clean-up 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants