Skip to content

qtwebengine: replace targetPlatform with hostPlatform#267292

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

qtwebengine: replace targetPlatform with hostPlatform#267292
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Nov 13, 2023 •
edited by ghost
Loading

Description of changes

stdenv.targetPlatform really shouldn't be used by software that doesn't generate or manipulate binaries.

I did a mass-replacement in #267229 and it affected eval for only two packages; one of them is qt6-qtwebengine, but it is affected only on Darwin. So I am breaking this out into a separate PR.

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/)
  • 23.11 Release Notes (or backporting 23.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.

@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: emacs Text editor 6.topic: printing Drivers, CUPS & Co. labels Nov 13, 2023
@ghost ghost marked this pull request as ready for review November 13, 2023 20:33
@ghost ghost requested a review from adisbladis as a code owner November 13, 2023 20:33
qtwebengine: replace targetPlatform with hostPlatform
91ac57c 
stdenv.targetPlatform really shouldn't be used by software that
doesn't generate or manipulate binaries.

I did a mass-replacement in
#267229 and it affected eval
for only two packages; one of them is qt6-qtwebengine, but it is
affected only on Darwin.  So I am breaking this out into a separate
PR.
@ghost ghost changed the title qtwebengine: s_targetPlatform_hostPlatform_ in non-compiler packages qtwebengine: replace targetPlatform with hostPlatform Nov 13, 2023
@github-actions github-actions bot removed 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: emacs Text editor 6.topic: printing Drivers, CUPS & Co. labels Nov 13, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Nov 13, 2023
@puetzk
Copy link
Copy Markdown
Contributor

puetzk commented Dec 4, 2023

I bet the difference on darwin is coming from

nixpkgs/pkgs/os-specific/darwin/apple-sdk-11.0/default.nix

Lines 66 to 69 in 7206143

targetPlatform = stdenv.targetPlatform // {
darwinMinVersion = "10.12";
darwinSdkVersion = "11.0";
};

which overrides darwinSdkVersion for targetPlatform but not hostPlatform. It seems like Qt was definitely aiming to get the higher CMAKE_OSX_DEPLOYMENT_TARGET = "11.0" set there; they went out of their way to override stdenv on darwin accordingly:

nixpkgs/pkgs/development/libraries/qt-6/default.nix

Line 30 in 91ac57c

stdenv = if stdenv.isDarwin then darwin.apple_sdk_11_0.stdenv else stdenv;

And this is presumably per the upstream platform support being macOS 11, 12, 13 but nothing older.

So, while this change seems logically correct (qtWebEngine isn't a cross-compiler and should use hostPlatform, I think darwin.apple_sdk_11_0 would need to be fixed to set hostPlatform as well to make that all work out.

Either that or qtwebengine should just be literally coding CMAKE_OSX_DEPLOYMENT_TARGET=11.0 (the minimum per upstream's requirements), and not trying to make different binaries per host revision. greping nixpkgs for CMAKE_OSX_DEPLOYMENT_TARGET seems to find that more often, and it would make for fewer distinct builds...

@puetzk
Copy link
Copy Markdown
Contributor

puetzk commented Dec 4, 2023

Also seems like

nixpkgs/pkgs/development/libraries/qt-6/default.nix

Lines 28 to 31 in 91ac57c

callPackage = self.newScope ({
inherit qtModule srcs python3;
stdenv = if stdenv.isDarwin then darwin.apple_sdk_11_0.stdenv else stdenv;
});
is perhaps not using apple_sdk_11_0 as intended (or at least not as documented).

https://nixos.org/manual/nixpkgs/unstable/#sec-darwin says

x86_64-darwin uses the 10.12 SDK by default, but some software is not compatible with that version of the SDK. In that case, the 11.0 SDK used by aarch64-darwin is available for use on x86_64-darwin. To use it, reference apple_sdk_11_0 instead of apple_sdk in your derivation and use pkgs.darwin.apple_sdk_11_0.callPackage instead of pkgs.callPackage. On Linux, this will have the same effect as pkgs.callPackage, so you can use pkgs.darwin.apple_sdk_11_0.callPackage regardless of platform.

So it seems like this perhaps should be using the whole apple_sdk_11_0.callPackage instead of its own callPackage with only the stdenv changes from 11.0, but all the other SDK packages left at 10.12?

I don't have any x86_64-darwin machine, though.

@puetzk
Copy link
Copy Markdown
Contributor

puetzk commented Dec 4, 2023

Tagging @wegank, since they're the one who put this all into qt6 during #226377 and probably know what they were trying to do (or at least have this platform...)

@wegank
Copy link
Copy Markdown
Member

wegank commented Dec 4, 2023 •
edited
Loading

So, while this change seems logically correct (qtWebEngine isn't a cross-compiler and should use hostPlatform, I think darwin.apple_sdk_11_0 would need to be fixed to set hostPlatform as well to make that all work out.

Yes, and this has actually been reported in #238993 (comment). So for now CMAKE_OSX_DEPLOYMENT_TARGET should probably just be hardcoded to 11.0 .

And this is presumably per the upstream platform support being macOS 11, 12, 13 but nothing older.

Yes, once again. A modern approach would be stdenv = if stdenv.isDarwin then overrideSDK stdenv "11.0" else stdenv, but this was introduced very recently. As a result, all Qt 6 applications were built with, for example, frameworks from darwin.apple_sdk_11_0.frameworks explicitly.

@ghost ghost closed this Jan 23, 2024
@ghost ghost deleted the pr/targetPlatform/qt6-qtwebengine branch January 23, 2024 06:50
@wegank wegank mentioned this pull request Jan 23, 2024
Merged
13 tasks
@Artturin Artturin mentioned this pull request Jan 29, 2024
Open
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adisbladis adisbladis Awaiting requested review from adisbladis

Assignees

No one assigned

Labels

10.rebuild-darwin: 11-100 This PR causes between 11 and 100 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.

2 participants

@puetzk @wegank