cxxopts: fix darwin build

[stephank]

Motivation for this change

Darwin Hydra failure: https://hydra.nixos.org/build/142025157/log

I've been trying to fix the Darwin build, and the change here solves the immediate error from the log above, however it then fails with:

building
build flags: -j8 -l8 SHELL=/nix/store/30njb8l701pwnm5ya749fh2cgyc2d70m-bash-4.4-p23/bin/bash
[ 37%] Building CXX object src/CMakeFiles/example.dir/example.cpp.o
error: unknown warning option '-Wsuggest-override'; did you mean '-Wshift-overflow'? [-Werror,-Wunknown-warning-option]

This appears to have been fixed upstream, but there is no new release. I'm hesitant to upgrade the unstable version used here, because it is linked to the version of ydotool, which is also Linux only. Cherry picking the change would also be a custom patch in this case. So we're stuck, hence I propose we flag it as Linux-only.

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

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.

[r-rmcgibbo]

Result of nixpkgs-review pr 122800 at 6f747a3e run on aarch64-linux 1

2 packages built successfully:

• cxxopts
• ydotool

1 suggestion:

• warning: name-and-version

  Did you mean to pass pname instead of name to mkDerivation?

  Near pkgs/development/libraries/cxxopts/default.nix:4:3:

    |
  4 |   name = "cxxopts";
    |   ^
  

  Near pkgs/development/libraries/cxxopts/default.nix:5:3:

    |
  5 |   version = "unstable-2020-12-14";
    |   ^
  

---

Result of nixpkgs-review pr 122800 at 6f747a3e run on x86_64-linux 1

2 packages built successfully:

• cxxopts
• ydotool

1 suggestion:

• warning: name-and-version

  Did you mean to pass pname instead of name to mkDerivation?

  Near pkgs/development/libraries/cxxopts/default.nix:4:3:

    |
  4 |   name = "cxxopts";
    |   ^
  

  Near pkgs/development/libraries/cxxopts/default.nix:5:3:

    |
  5 |   version = "unstable-2020-12-14";
    |   ^
  

[dotlambda]

Maybe you can ask upstream to make a release with those fixes?

Anyway, I think we should rather mark it as broken.

[dotlambda]

Why not provide an alternative cmakeDir?

[stephank]

This just seemed more concise and doesn't fail. I think an alternate dir would look like:

{
  dontUseCmakeBuildDir = true;
  preConfigure = ''
    mkdir _build
    cd _build
    cmakeDir=..
  '';
}

Maybe you can ask upstream to make a release with those fixes?

I'm not familiar with these packages to know what that would do with ydotool.

Anyway, I think we should rather mark it as broken.

Good point, I made a change for this.

[dotlambda]

I think an alternate dir would look like

Never mind, yours looks definitely cleaner.

I'm not familiar with these packages to know what that would do with ydotool.

Let's hope @WilliButz can give some advice.

[jonringer]

{
  dontUseCmakeBuildDir = true;
  preConfigure = ''
    mkdir _build
    cd _build
    cmakeDir=..
  '';
}

That's essentially what the cmake hook does anyway. I think dontUseCmakeBuildDir = true; is fine. Shouldn't affect installation.... dependning on upstream's logic.

[spease]

You might be able to get rid of the error by setting cmake flag CXXOPTS_BUILD_EXAMPLES to OFF to not build the example code. This should probably have been done anyway.

Also, the pname warning was another error on my part.

I don’t understand where the build directory already existing error is coming from that is prompting this change?

[stephank]

I wasn’t really clear with the quoted output, but everything is compiled with that compiler flag, not just examples.

The conflict is because the source dir contains a file BUILD, and the Nix cmake hook tries to create a build directory.

[spease]

Yeah, it might still come up with the tests, but I don’t think the examples should be built by default.

The decision of whether to build the examples and the tests is implicitly controlled by

if (CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR)
    set(CXXOPTS_STANDALONE_PROJECT ON)
endif()

If dontUseCmakeBuildDir is triggering that, that may be why the error started occurring, and the alt dir suggestion might avoid it. Though I agree that patching would be the best solution if it doesn’t break ydotool; it would be preferable to use the more succinct syntax and have the unit tests run on all platforms.

[spease]

This change will allow cxxopts to build on macOS using the current rev pinned in master:

-  cmakeFlags = lib.optional enableUnicodeHelp [ "-DCXXOPTS_USE_UNICODE_HELP=TRUE" ];
+  cmakeFlags = [ "-DCXXOPTS_BUILD_EXAMPLES=OFF" ]
+    ++ lib.optional enableUnicodeHelp "-DCXXOPTS_USE_UNICODE_HELP=TRUE"
+    # Due to -Wsuggest-override, remove when cxxopts is updated
+    ++ lib.optional stdenv.isDarwin "-DCXXOPTS_ENABLE_WARNINGS=OFF";

The crucial line being:

++ lib.optional stdenv.isDarwin "-DCXXOPTS_ENABLE_WARNINGS=OFF";

Turning off building the examples should be unnecessary, but I think I should have done this the first time and it eliminates 'example' from being unnecessarily generated or included in other package's dependencies.

I did look into trying to set CMAKE_CXX_FLAGS or CXXFLAGS manually to all the flags that cxxopts would have set in the current version sans the suggest-override flag that the current version of clang in nix doesn't support, but I couldn't figure out the proper syntax or find a working example.

Obviously it's not great, but since it will continue to build on other platforms with warnings enabled and I don't expect there to be many platform differences, and since upstream maintainers are really responsible for the sort of warnings it would generate, I think it's better than breaking it on macOS.

But I agree the ideal would be to update it to include the patch. I bumped it myself locally and it built fine, so if ydotool can do that, I would favor bumping the version to switching off the warnings.

[stephank]

Looks good, I've updated the PR with your changes. :)

[dotlambda]

@ofborg build cxxopts

[dotlambda]

Why did you remove this?

[stephank]

I was asked to remove this in another review. Assumed it meant the same when nothing was specified?

Regardless, it probably shouldn't be in this PR. I'll remove it.

[jonringer]

probably because it's the default. But i don't think it detracts from readability since meta is at the bottom of the file anyway.

[dotlambda]

@ofborg build cxxopts

[jonringer]

LGTM

Result of nixpkgs-review pr 122800 run on x86_64-linux 1

2 packages built:

• cxxopts
• ydotool