Skip to content

stdenv: start deprecating non-list cmakeFlags#180067

Merged
roberth merged 2 commits intoNixOS:stagingfrom
Artturin:depcmakestring
Jul 5, 2022
Merged

stdenv: start deprecating non-list cmakeFlags#180067
roberth merged 2 commits intoNixOS:stagingfrom
Artturin:depcmakestring

Conversation

@Artturin
Copy link
Copy Markdown
Member

@Artturin Artturin commented Jul 4, 2022 •
edited by samueldr
Loading

the motivation for this is to simplify stdenv and ease the job of
reviewers due to them needing to tell contributors about the defacto
rule that cmakeFlags should be a list of strings

Description of changes

continuation of #173172
this is why I made the deprecation release of them 23.05

meson will be done in a pr after this one is merged

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@Artturin Artturin requested a review from SuperSandro2000 July 4, 2022 01:35
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jul 4, 2022
@Artturin Artturin mentioned this pull request Jul 4, 2022
Merged
10 tasks
@Artturin Artturin marked this pull request as ready for review July 4, 2022 01:55
@Artturin Artturin mentioned this pull request Jul 4, 2022
Merged
13 tasks
@Artturin Artturin requested review from Mindavi and roberth July 4, 2022 02:05
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. labels Jul 4, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must 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: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jul 4, 2022
SuperSandro2000
SuperSandro2000 reviewed Jul 4, 2022
View reviewed changes
magnetophon
magnetophon approved these changes Jul 4, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@magnetophon magnetophon left a comment

Choose a reason for hiding this comment

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

At least the faust part LGTM.

@Artturin
Copy link
Copy Markdown
Member Author

Artturin commented Jul 4, 2022 

$ gco depcmakestring
Switched to branch 'depcmakestring'
Your branch is up to date with 'origin/depcmakestring'.
$ nix eval ".#pkgsCross.aarch64-multiplatform.h3.cmakeFlags"
[ "-DBUILD_SHARED_LIBS=ON" "-DENABLE_LINTING=OFF" "-DCMAKE_SYSTEM_NAME=Linux" "-DCMAKE_SYSTEM_PROCESSOR=aarch64" "-DCMAKE_HOST_SYSTEM_NAME=Linux" "-DCMAKE_HOST_SYSTEM_PROCESSOR=x86_64" ]
$ nix eval ".#h3.cmakeFlags"
[ "-DBUILD_SHARED_LIBS=ON" "-DENABLE_LINTING=OFF" ]
$ gcm
Switched to branch 'master'
Your branch is up to date with 'upstream/master'.
$ nix eval ".#pkgsCross.aarch64-multiplatform.h3.cmakeFlags"
[ "-DBUILD_SHARED_LIBS=ON" "-DENABLE_LINTING=OFF" "-DCMAKE_SYSTEM_NAME=Linux" "-DCMAKE_SYSTEM_PROCESSOR=aarch64" "-DCMAKE_HOST_SYSTEM_NAME=Linux" "-DCMAKE_HOST_SYSTEM_PROCESSOR=x86_64" ]
$ nix eval ".#h3.cmakeFlags"
[ "-DBUILD_SHARED_LIBS=ON" "-DENABLE_LINTING=OFF" ]

@ofborg ofborg bot requested a review from magnetophon July 4, 2022 13:16
magnetophon
magnetophon approved these changes Jul 4, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@magnetophon magnetophon left a comment

Choose a reason for hiding this comment

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

Not sure why my review was requested again, but I'll appease ofborg.

@ofborg ofborg bot requested a review from magnetophon July 4, 2022 13:43
@magnetophon
Copy link
Copy Markdown
Member

magnetophon commented Jul 4, 2022

I'm being stalked by ofborg! ;)

@bobby285271 bobby285271 added 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Jul 4, 2022
roberth
roberth reviewed Jul 4, 2022
View reviewed changes
@ckiee ckiee removed their request for review July 4, 2022 14:47
@ofborg ofborg bot requested a review from ckiee July 4, 2022 17:14
@Mindavi Mindavi removed the request for review from ckiee July 4, 2022 18:12
Mindavi
Mindavi reviewed Jul 4, 2022
View reviewed changes
jtojnar
jtojnar reviewed Jul 4, 2022
View reviewed changes
pkgs/stdenv/generic/make-derivation.nix Outdated
Copy link
Copy Markdown
Member

@jtojnar jtojnar Jul 4, 2022

Choose a reason for hiding this comment

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

The indentation here is weird – almost looks like it belongs to the else branch.

Copy link
Copy Markdown
Member

@jtojnar jtojnar Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe something like the following to get rid of the alignment, which looks ugly with proportional fonts:

blagh

      cmakeFlags =
        let
          inherit (lib) optionals;

          explicitFlags =
            if lib.isString cmakeFlags then
              lib.warn
                "String 'cmakeFlags' is deprecated and will be removed in release 23.05. Please use a list of strings. Derivation name: ${derivationArg.name}, file: ${pos.file or "unknown file"}"
                [cmakeFlags]
            else if cmakeFlags == null then
              lib.warn
                "Null 'cmakeFlags' is deprecated and will be removed in release 23.05. Please use a empty list instead '[]'. Derivation name: ${derivationArg.name}, file: ${pos.file or "unknown file"}"
                []
            else
              cmakeFlags;

          crossFlags = [
            "-DCMAKE_SYSTEM_NAME=${lib.findFirst lib.isString "Generic" (lib.optional (!stdenv.hostPlatform.isRedox) stdenv.hostPlatform.uname.system)}"
          ] ++ optionals (stdenv.hostPlatform.uname.processor != null) [
            "-DCMAKE_SYSTEM_PROCESSOR=${stdenv.hostPlatform.uname.processor}"
          ] ++ optionals (stdenv.hostPlatform.uname.release != null) [
            "-DCMAKE_SYSTEM_VERSION=${stdenv.hostPlatform.uname.release}"
          ] ++ optionals (stdenv.hostPlatform.isDarwin) [
            "-DCMAKE_OSX_ARCHITECTURES=${stdenv.hostPlatform.darwinArch}"
          ] ++ optionals (stdenv.buildPlatform.uname.system != null) [
            "-DCMAKE_HOST_SYSTEM_NAME=${stdenv.buildPlatform.uname.system}"
          ] ++ optionals (stdenv.buildPlatform.uname.processor != null) [
            "-DCMAKE_HOST_SYSTEM_PROCESSOR=${stdenv.buildPlatform.uname.processor}"
          ] ++ optionals (stdenv.buildPlatform.uname.release != null) [
            "-DCMAKE_HOST_SYSTEM_VERSION=${stdenv.buildPlatform.uname.release}"
          ];
        in
          explicitFlags
          ++ optionals (stdenv.hostPlatform != stdenv.buildPlatform) crossFlags;

Copy link
Copy Markdown
Member Author

@Artturin Artturin Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The lists were on the different lines before Sandro suggested they be on the same line

Also luckily everyone uses monospace fonts for programming

Will try applying your suggestion

Copy link
Copy Markdown
Member

@ckiee ckiee Jul 5, 2022

Choose a reason for hiding this comment

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

Also luckily everyone uses monospace fonts for programming

There's a good chance that they aren't if they noticed it :P

Copy link
Copy Markdown
Member Author

@Artturin Artturin Jul 5, 2022

Choose a reason for hiding this comment

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

applied the suggestion but reduced the vertical space need

Copy link
Copy Markdown
Member

@SuperSandro2000 SuperSandro2000 Jul 5, 2022

Choose a reason for hiding this comment

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

Also luckily everyone uses monospace fonts for programming

yes, JetBrains Mono

I don't like really like the code below. Its just a giant block and before we could easily see what happens but we bikesheeded enough here.

          crossFlags = [
            "-DCMAKE_SYSTEM_NAME=${lib.findFirst lib.isString "Generic" (lib.optional (!stdenv.hostPlatform.isRedox) stdenv.hostPlatform.uname.system)}"
          ] ++ optionals (stdenv.hostPlatform.uname.processor != null) [
            "-DCMAKE_SYSTEM_PROCESSOR=${stdenv.hostPlatform.uname.processor}"
          ] ++ optionals (stdenv.hostPlatform.uname.release != null) [
            "-DCMAKE_SYSTEM_VERSION=${stdenv.hostPlatform.uname.release}"
          ] ++ optionals (stdenv.hostPlatform.isDarwin) [
            "-DCMAKE_OSX_ARCHITECTURES=${stdenv.hostPlatform.darwinArch}"
          ] ++ optionals (stdenv.buildPlatform.uname.system != null) [
            "-DCMAKE_HOST_SYSTEM_NAME=${stdenv.buildPlatform.uname.system}"
          ] ++ optionals (stdenv.buildPlatform.uname.processor != null) [
            "-DCMAKE_HOST_SYSTEM_PROCESSOR=${stdenv.buildPlatform.uname.processor}"
          ] ++ optionals (stdenv.buildPlatform.uname.release != null) [
            "-DCMAKE_HOST_SYSTEM_VERSION=${stdenv.buildPlatform.uname.release}"
          ];

Artturin added 2 commits July 5, 2022 04:05
@Artturin
stdenv: start deprecating non-list cmakeFlags
3173c3b 
the motivation for this is to simplify stdenv and ease the job of
reviewers due to them needing to tell contributors about the defacto
rule that cmakeFlags should be a list of strings
@Artturin
treewide: convert string cmakeFlags to list of strings
732f1d8
@Artturin
Copy link
Copy Markdown
Member Author

Artturin commented Jul 5, 2022 •
edited
Loading

no more style changes please :P

@ofborg ofborg bot requested a review from ckiee July 5, 2022 02:46
roberth
roberth reviewed Jul 5, 2022
View reviewed changes
@roberth roberth merged commit d295294 into NixOS:staging Jul 5, 2022
@Artturin Artturin deleted the depcmakestring branch July 5, 2022 19:31
@Artturin
Copy link
Copy Markdown
Member Author

Artturin commented Jul 6, 2022

#180302

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

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@roberth roberth roberth approved these changes

@jtojnar jtojnar jtojnar approved these changes

@Mindavi Mindavi Mindavi approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@pmahoney pmahoney Awaiting requested review from pmahoney

@puffnfresh puffnfresh Awaiting requested review from puffnfresh

@pasqui23 pasqui23 Awaiting requested review from pasqui23

@magnetophon magnetophon Awaiting requested review from magnetophon

@ckiee ckiee Awaiting requested review from ckiee

+1 more reviewer

@trofi trofi trofi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Artturin @magnetophon @trofi @roberth @jtojnar @SuperSandro2000 @Mindavi @ckiee @bobby285271