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

qt6.qtModule: support finalAttrs #352466

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

qt6.qtModule: support finalAttrs #352466

wants to merge 3 commits into from

Conversation

Artturin
Copy link
Member

@Artturin Artturin commented Oct 30, 2024 •
edited
Loading

Based on https://www.github.com/NixOS/nixpkgs/pull/331398

Overriding works as it should

nix-repl> qt6.qtbase.overrideAttrs (previousAttrs: { nativeBuildInputs = previousAttrs.nativeBuildInputs ++ [ pkgs.hello ]; })
«derivation /nix/store/wyh7snh6nf8mnnp9saffc5sjaqid32wp-qtbase-6.8.0.drv»

nix-repl> p = qt6.qtbase.overrideAttrs (previousAttrs: { nativeBuildInputs = previousAttrs.nativeBuildInputs ++ [ pkgs.hello ]; })

nix-repl> p.nativeBuildInputs
[
  «derivation /nix/store/yp35rww8ybc71bmgdq9b27a7hz2wca99-bison-3.8.2.drv»
  «derivation /nix/store/4rzliqardkl299845js8xwy4p0l4w0bn-flex-2.6.4.drv»
  «derivation /nix/store/rvam632pb8a7j17w3nmai4mg91abzaw1-gperf-3.1.drv»
  «derivation /nix/store/xxm2ks3fdcqfgfiajsldfrz7vz2rjwr4-lndir-1.0.5.drv»
  «derivation /nix/store/dd75caqmnsk3g269hkgsbnbj5gn1xcd3-perl-5.40.0.drv»
  «derivation /nix/store/6kqfi7ndxaiwy25glcc97fkx6dcp2722-pkg-config-wrapper-0.29.2.drv»
  «derivation /nix/store/7b1d2z8lh6a4hjd57j6rjm155djjzddz-which-2.21.drv»
  «derivation /nix/store/s7wkvim8lplr5k4zf0k083ivhwn0im2l-cmake-3.29.6.drv»
  «derivation /nix/store/rbjl8vphyrn9jwaqm7545qwcvwac4jrv-xmlstarlet-1.6.1.drv»
  «derivation /nix/store/9pv5plcg55hln8ihsh4gchhj284sw9q6-ninja-1.12.1.drv»
  «derivation /nix/store/161hd6k485dvr9cx02ccnc1x4n9fzix6-hello-2.12.1.drv»
]

nix-repl> p = qt6.qtbase.overrideAttrs (previousAttrs: { dontWrapQtApps = false; })

nix-repl> p.dontWrapQtApps
false

nix-repl> qt6.qtbase.dontWrapQtApps
true

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Artturin Artturin mentioned this pull request Oct 30, 2024
Merged
@Artturin Artturin requested review from emilazy and K900 October 30, 2024 21:30
@emilazy
Copy link
Member

emilazy commented Oct 30, 2024

I believe @K900 expressed a desire to kill this off entirely; perhaps that would be better? Though the added complexity here doesn’t look so bad.

emilazy
emilazy approved these changes Oct 30, 2024
View reviewed changes
Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn’t notice the plugins change. This seems fine as a way to achieve that for now, but I’ll let @K900 take a look. I think we could avoid needing to change qtModule by just giving qtwayland a dependency on itself? But I guess overrides would act slightly differently.

@Artturin
Copy link
Member Author

Artturin commented Oct 30, 2024 •
edited
Loading

Added to qtbase too

@Artturin Artturin force-pushed the qtmodulefinalattrs branch 2 times, most recently from 7a58349 to f33aa53 Compare October 31, 2024 02:40
Artturin
Artturin commented Oct 31, 2024
View reviewed changes
pkgs/development/libraries/qt-6/modules/qtwayland.nix
}

passthru = {
plugins = runCommand "${finalAttrs.finalPackage.name}-only-plugins" { } ''
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be abstracted away in to qtModule and added with a hasQtPlugins = true

K900
K900 reviewed Oct 31, 2024
View reviewed changes
pkgs/development/libraries/qt-6/modules/qtbase.nix
passthru = {
plugins = runCommand "${finalAttrs.finalPackage.name}-only-plugins" { } ''
mkdir -p "$out/lib/qt-6/plugins"
ln -s "${finalAttrs.finalPackage}/lib/qt-6/plugins" "$out/lib/qt-6/plugins"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. It's basically just here for wrapQtAppsHook?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now yep unless others have more ideas for their use

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The desktop environment modules could install it? Since I assume they install full qtwayland currently. Though I guess that would no longer be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Desktop modules don't care if we pull in some cmake modules or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this either, could have been a separate output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qtwayland exposes an API for writing Wayland compositors that at least one thing in the tree uses, so we do risk undeclared dependencies if we propagate all of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a risk we can take.

Copy link
Member Author

@Artturin Artturin Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move the symlinking directly in to the wrapper if this is not wanted, if we propagate just out as it currently is we might propagating a partially broken package

Please remember env bloat is bad

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can just fix splitting $dev properly, and then have the hooks only propagate $out?

Only if there's a way to make the qt generated cmake files happy, last time I tried they blew up fantastically.

We used to patch cmake with for this purpose:

commit 16a7c7b31cef945a9e66e07283ed45cb8ff8e216 (HEAD -> nixpkgs)
Author: Nick Cao <[email protected]>
Date:   Tue Apr 11 23:44:15 2023 +0800

    cmake always export install file with absolute path

diff --git a/Source/cmExportInstallFileGenerator.cxx b/Source/cmExportInstallFileGenerator.cxx
index 195737b9fc..8d7d37e6a5 100644
--- a/Source/cmExportInstallFileGenerator.cxx
+++ b/Source/cmExportInstallFileGenerator.cxx
@@ -203,7 +203,7 @@ void cmExportInstallFileGenerator::GenerateImportPrefix(std::ostream& os)
     this->IEGen->GetLocalGenerator()->GetMakefile()->GetSafeDefinition(
       "CMAKE_INSTALL_PREFIX");
   std::string const& expDest = this->IEGen->GetDestination();
-  if (cmSystemTools::FileIsFullPath(expDest)) {
+  if (true) {
     // The export file is being installed to an absolute path so the
     // package is not relocatable.  Use the configured install prefix.
     /* clang-format off */

Copy link
Member Author

@Artturin Artturin Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the plugin symlinking directly to #352419

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 31, 2024
@Artturin
qt6.{qtwayland,qtbase}: Add passthru.plugins
58712c3 
So we can propagate plugins in `wrapQtAppsHook` without
polluting the environment
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 31, 2024
@Artturin Artturin marked this pull request as draft November 5, 2024 21:21
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 10, 2024
@9999years 9999years mentioned this pull request Jan 14, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilazy emilazy emilazy approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@K900 K900 K900 left review comments

@NickCao NickCao Awaiting requested review from NickCao

@ttuegel ttuegel Awaiting requested review from ttuegel

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Artturin @emilazy @K900 @SuperSandro2000 @NickCao @wegank