Skip to content

qt5.qtbase: prepare for structuredAttrs#472655

Open
SFrijters wants to merge 1 commit intoNixOS:stagingfrom
SFrijters:qt5-base-replacevars-structuredattrs
Open

qt5.qtbase: prepare for structuredAttrs#472655
SFrijters wants to merge 1 commit intoNixOS:stagingfrom
SFrijters:qt5-base-replacevars-structuredattrs

Conversation

@SFrijters
Copy link
Member

@SFrijters SFrijters commented Dec 20, 2025 •
edited
Loading

This fixes the hook for downstream users when __structuredAttrs are enabled.

#237216

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@SFrijters SFrijters marked this pull request as draft December 20, 2025 10:00
@SFrijters SFrijters mentioned this pull request Dec 20, 2025
Open
4 tasks
@SFrijters SFrijters force-pushed the qt5-base-replacevars-structuredattrs branch from a442e27 to 523f168 Compare December 20, 2025 10:06
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. labels Dec 20, 2025
@SFrijters SFrijters force-pushed the qt5-base-replacevars-structuredattrs branch from 523f168 to ef2cbcd Compare December 22, 2025 23:55
@SFrijters SFrijters changed the title qt5.qtbase: use replaceVars for setup hook qt5.qtbase: prepare for structuredAttrs Dec 22, 2025
SFrijters
SFrijters commented Dec 22, 2025
View reviewed changes
@SFrijters SFrijters marked this pull request as ready for review December 22, 2025 23:57
@SFrijters SFrijters force-pushed the qt5-base-replacevars-structuredattrs branch from ef2cbcd to cfe665d Compare December 23, 2025 00:01
@SFrijters SFrijters marked this pull request as draft December 23, 2025 00:01
@SFrijters
Copy link
Member Author

SFrijters commented Dec 23, 2025

Ugh, back to draft, for some variants I'm getting Project ERROR: /build/qtbase-bebdfd5/examples/qpa/windows/windows.pro installs target to unexpected location and related errors.

@SFrijters SFrijters force-pushed the qt5-base-replacevars-structuredattrs branch 7 times, most recently from 9c64598 to ce7b405 Compare December 25, 2025 01:05
@SFrijters
Copy link
Member Author

SFrijters commented Dec 25, 2025

lib.optional considered harmful... wasted a lot of time trying to get substitutions fixed, but the last error was actually lib.optional instead of lib.optionals invalidating configureFlags :(

@SFrijters SFrijters marked this pull request as ready for review December 25, 2025 01:12
@SFrijters
Copy link
Member Author

SFrijters commented Dec 25, 2025 •
edited
Loading

New problem: build of qca fails because /nix/store/v6z6z60r94fmrg0gli90fc4frkgvg5p4-qtbase-5.15.18-dev/lib/cmake/Qt5Core/Qt5CoreConfigExtras.cmake refers to /nix/store/gilra199wlk01wlr09iss4mpcn63ra52-qtbase-5.15.18-bin/bin/qmake but it should refer to the dev output instead: /nix/store/v6z6z60r94fmrg0gli90fc4frkgvg5p4-qtbase-5.15.18-dev/bin/qmake exists.

@K900 K900 marked this pull request as draft December 25, 2025 07:24
@SFrijters
Copy link
Member Author

SFrijters commented Dec 27, 2025

I have fixed the problem locally in a hacky way by rewriting the paths in fixup, I just have to do it properly before I re-open this for review.

@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 25, 2026
@SFrijters SFrijters force-pushed the qt5-base-replacevars-structuredattrs branch from ce7b405 to 314e85d Compare January 30, 2026 16:41
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 30, 2026
@SFrijters SFrijters force-pushed the qt5-base-replacevars-structuredattrs branch from 314e85d to 7a05aef Compare January 30, 2026 17:26
@SFrijters
Copy link
Member Author

SFrijters commented Jan 30, 2026

Setting up devTools correctly seems to have been the problem on my previous round of testing. With this commit picked onto master I could build an example Qt5 program, and the cmake files look good now.

@SFrijters SFrijters marked this pull request as ready for review January 30, 2026 17:29
@SFrijters
Copy link
Member Author

SFrijters commented Feb 2, 2026

@K900 Since you reviewed my qt6 __structuredAttrs PR, could you take a look at this one as well?

@jopejoe1 jopejoe1 requested a review from a team February 19, 2026 15:25
@SFrijters
Copy link
Member Author

SFrijters commented Feb 25, 2026

Need to be extra careful after #493988 - but I don't think the same issue applies since makeSetupHook doesn't have a dev output.

@SFrijters SFrijters force-pushed the qt5-base-replacevars-structuredattrs branch from 7a05aef to 74863b6 Compare February 25, 2026 11:12
@SFrijters
Copy link
Member Author

SFrijters commented Feb 25, 2026 •
edited
Loading

Rebased to fix a small merge conflict with 8198334 / #492070.

doronbehar
doronbehar reviewed Mar 2, 2026
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Changes look pretty good overall, but it is hard to review this due to the tangent issues this PR touches, all part of a single commit.

pkgs/development/libraries/qt-5/modules/qtbase.nix
Comment on lines +157 to +159
++ lib.optionals libGLSupported [
libGL
]
Copy link
Contributor

@doronbehar doronbehar Mar 2, 2026

Choose a reason for hiding this comment

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

This kind of diff hunk is good I think, but I find it tangent to the main point of this PR. Could you please avoid these lib.{optional -> optionals} changes and focus on the actual changes of the PR? Or alternatively, separate it to a different commit?

pkgs/development/libraries/qt-5/modules/qtbase.nix
substituteInPlace configure --replace /bin/pwd pwd
substituteInPlace src/corelib/global/global.pri --replace /bin/ls ${coreutils}/bin/ls
substituteInPlace configure --replace-fail /bin/pwd pwd
substituteInPlace src/corelib/global/global.pri --replace-fail /bin/ls ${coreutils}/bin/ls
Copy link
Contributor

@doronbehar doronbehar Mar 2, 2026

Choose a reason for hiding this comment

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

This change too is blessed, but should be a different commit.

pkgs/development/libraries/qt-5/modules/qtbase.nix
"-device ${qtPlatformCross stdenv.hostPlatform}"
"-device-option CROSS_COMPILE=${stdenv.cc.targetPrefix}"
"-device"
"${qtPlatformCross stdenv.hostPlatform}"
Copy link
Contributor

@doronbehar doronbehar Mar 2, 2026

Choose a reason for hiding this comment

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

Nit:

Suggested change
"${qtPlatformCross stdenv.hostPlatform}"
(qtPlatformCross stdenv.hostPlatform)

pkgs/development/libraries/qt-5/modules/qtbase.nix
postFixup = ''
# Don't retain build-time dependencies like gdb.
sed '/QMAKE_DEFAULT_.*DIRS/ d' -i $dev/mkspecs/qconfig.pri
Copy link
Contributor

@doronbehar doronbehar Mar 2, 2026

Choose a reason for hiding this comment

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

Another unrelated change.

pkgs/development/libraries/qt-5/qtModule.nix
done
fi
${lib.optionalString (lib.hasAttr "devTools" args) ''devTools="${lib.concatStringsSep " " args.devTools}"''}
Copy link
Contributor

@doronbehar doronbehar Mar 2, 2026

Choose a reason for hiding this comment

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

Could you explain how come this is needed all of a sudden?

pkgs/development/libraries/qt-5/modules/qtbase.nix
Comment on lines +573 to +589
setupHook =
let
hook = makeSetupHook {
name = "qtbase5-setup-hook";
substitutions = {
inherit
qtPluginPrefix
qtQmlPrefix
qtDocPrefix
fix_qt_builtin_paths
fix_qt_module_paths
;
debug = debugSymbols;
};
} ../hooks/qtbase-setup-hook.sh;
in
"${hook}/nix-support/setup-hook";
Copy link
Contributor

@doronbehar doronbehar Mar 2, 2026

Choose a reason for hiding this comment

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

How come it worked before without these substitutions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@doronbehar doronbehar doronbehar left review comments

@bkchr bkchr Awaiting requested review from bkchr

@K900 K900 Awaiting requested review from K900

@NickCao NickCao Awaiting requested review from NickCao

@OPNA2608 OPNA2608 Awaiting requested review from OPNA2608

@qknight qknight Awaiting requested review from qknight

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@ttuegel ttuegel Awaiting requested review from ttuegel

Assignees

No one assigned

Labels

10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SFrijters @doronbehar