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

lxqt-build-tools: fix darwin detection #123400

Merged
merged 1 commit into from May 18, 2021
Merged

Conversation

stephank
Copy link
Contributor

Motivation for this change

ZHF: #122042
cc @NixOS/nixos-release-managers

This package detects Darwin via the 'AppleClang' identifier that only applies to Xcode; Nix clang identifies as 'Clang'. This causes dependants to fail linking, for example: https://hydra.nixos.org/build/142959638/nixlog/1

This patch is a stop-gap solution, I think. i don't think it's appropriate to upstream, but I also don't know what the correct solution is.

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.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 17, 2021
@ofborg ofborg bot requested a review from romildo May 17, 2021 19:36
Copy link
Contributor

@romildo romildo left a comment

Choose a reason for hiding this comment

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

I do not have/use darwin, therefore I cannot test. But it looks good to me.

@risicle
Copy link
Contributor

risicle commented May 17, 2021

Could this not have been done with substituteInPlace?

@risicle
Copy link
Contributor

risicle commented May 17, 2021

Certainly lxqt.compton-conf and lxqt.libsysstat now build for me on macos 10.15.

@stephank
Copy link
Contributor Author

stephank commented May 18, 2021

I wasn't sure what is preferred. I updated the PR. 👍

@jonringer
Copy link
Contributor

@GrahamcOfBorg build lxqt-build-tools

Comment on lines 27 to 30
postPatch = lib.optionalString stdenv.isDarwin ''
substituteInPlace cmake/modules/LXQtCompilerSettings.cmake \
--replace AppleClang Clang
'';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
postPatch = lib.optionalString stdenv.isDarwin ''
substituteInPlace cmake/modules/LXQtCompilerSettings.cmake \
--replace AppleClang Clang
'';
postPatch = ''
substituteInPlace cmake/modules/LXQtCompilerSettings.cmake \
--replace AppleClang Clang
'';

Otherwise we might miss it when updating on Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

I'm now a little worried in general that this kind of substitution may break something when upstream these conditions in the cmake code. Then again, that'll probably result in a broken Darwin build, or even a broken build everywhere. So I guess it's fine?

@SuperSandro2000
Copy link
Member

@ofborg build lxqt.lxqt-build-tools

@jonringer jonringer merged commit 3e6d8a7 into NixOS:master May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants