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

[WIP/RFC] Make cmakeFlags more ergonomic #17886

Closed

Conversation

aneeshusa
Copy link
Contributor

@aneeshusa aneeshusa commented Aug 21, 2016

Motivation for this change

Similar to #15799, but with a smaller scope (can change everything at one time so no need for deprecation) and with an even nicer interface.

Things done
  • Tested using sandboxing
    (nix.useChroot on NixOS,
    or option build-use-chroot in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

There are three main parts to this change:

  • Support a new setupHookFunc in mkDerivation
  • Using attribute sets for cmakeFlags in Nix
  • Passing cmakeFlags to the builder more smartly

== Introducing setupHookFunc

The setupHook attribute allows a derivation to register arbitrary
modifications to run in the context of downstream dependents' builds.

To enable similar modifications to be made to the attributes passed
to mkDerivation, teach mkDerivation a new trick: setupHookFunc.
A derivation can set setupHookFunc to a Nix function
which will be called for downstream dependencies
(TODO using the same logic as the setupHook inclusion),
and which takes the attribute set which was passed to mkDerivation
and returns a new attribute set with any desired changes.

These will stack (TODO: resolution order?) for composability,
and having these live alongside the original derivation
avoids cluttering mkDerivation with cmake-specific code
(which was done in an earlier iteration).

TODO: benchmark

== Using Attribute Sets for cmakeFlags

Almost all flags passed to cmakeFlags are setting CMake cache options
with "-D=" or "-D:=".
Because the CMake cache is essentially a set of keys and values,
model cmakeFlags as an attribute set in Nix as well.

The cmake setupHookFunc will take a cmakeFlags set
and convert each key/value pair to an appropriate "-D" option for cmake.
String values are used as is, while integers are converted to strings
and booleans are converted to "ON" and "OFF".
This presents the following benefits:

  • Removal of "-D" visual noise everywhere for ease of reading.
  • The = operater is literal, allowing extra whitespace for more ease
    of reading.
  • CMake options are now first class Nix values that can be operated on
    directly with the full strength of the Nix language.
    For example, boolean values can be used directly because the
    serialization logic is consolidated into the cmake setupHookFunc,
    getting rid of helpers like edf
    and making it easier to re-use feature flags like pythonSupport.
  • Sets automatically sort options, improving cache re-use.
  • An override which updates a CMake flag option will only incur
    (amortized) O(1) cost with a set, as opposed to O(n) cost with an
    (unsorted) list, which would require a full linear search.
    It's also shorter to write: just use //.

A few other CMake flag types are supported:

  • Setting a value to null will cause its key to be unset via "-U".
  • The generator key is mapped to the "-G" option.
  • The extraArgs key is an escape hatch; any flags in this list of
    strings are not pre-processed, but concatenated to the generated list.

Because setupHookFuncs are processed during mkDerivation,
when overriding cmakeFlags overrideAttrs should be used instead of
overrideDerivation so the cmake setupHookFunc has a chance to rerun.

== Passing cmakeFlags to the builder more smartly

Nix's regular conversion of lists to an environment variable
simply concatenates the contents with spaces,
which breaks if any of the flags themselves contain spaces.
cmakeFlagsArray was added as a way to get around this limitation,
but is unsatisfactory to use because items must be appended via bash
in a preConfigure hook.

Instead, pass the list to the builder in a smarter way:
instead of relying on Nix's conversion to a string,
perform our own conversion by escaping double quotes,
double quoting each item, and passing the flags as a Bash array,
which is hydrated via eval.

This handles flags with spaces, newlines and other whitespace,
as well as double quotes, faithfully.

TODO: does it handle eg some_var="$out/blah"
Make the list available during preConfigure as a bash array, so any
dynamic modifications to the CMake flags can be done there.

These changes make also cmakeFlagsArray redundant, so it has been
removed and replaced in all cases with cmakeFlags.


Notes/questions:

  • I'd like to know how to document this (as a breaking change). Since we don't have a good strategy for deprecation, I'd like to avoid that entirely.
  • I've only updated some of the packages (namely those I've use locally); if this is a desired change than I can figure out the best way to update all the rest of the packages, too. I made these changes by hand and did compile a few, but I'd rather not compile all of these on my laptop. Can this patchset be run on Hydra to catch any inadvertent breakage?

@mention-bot
Copy link

@aneeshusa, thanks for your PR! By analyzing the annotation information on this pull request, we identified @christopherpoole, @jraygauthier and @vcunat to be potential reviewers

@aneeshusa aneeshusa force-pushed the make-cmakeFlags-more-ergonomic branch from 86bd73d to b64b29d Compare August 21, 2016 16:55
cmakeFlags = [
"-DSC_WII=OFF"
"-DSC_EL=${if useSCEL then "ON" else "OFF"}"
];
Copy link
Member

Choose a reason for hiding this comment

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

Why not this?

cmakeFlags = {
  SC_WII = false;
  SC_EL = useSCEL;
};

@aneeshusa
Copy link
Contributor Author

@rasendubi there are over 300 files that use cmakeFlags, I want to get some feedback before updating the remaining files (which should be fairly mechanical). This is by nature a big PR and I'd like to ensure this is a wanted change before sinking that much time into it.

@rasendubi
Copy link
Member

I'm in favor of this, but I'm not a core contributor, so I think my voice has little value.

сс other Nix developers who have changed cmake in the near past: @ttuegel @abbradar @vcunat @nbp

@bjornfor
Copy link
Contributor

I think cmakeFlags should be a list, not a set. A set feels like wrong abstraction to me: a command-line interface is a list of arguments, not a set. (How to specify "foo -v -v" (double verbose) if args is a set?). Just my opinion.

@gebner
Copy link
Member

gebner commented Aug 25, 2016

@bjornfor The set is for the -DKEY=value options, I don't think it makes sense to duplicate such an option, as the second one will just overwrite the first one IIRC.

FWIW, I find the { KEY = "value"; } version much more readable than the -DKEY=value one, and it's also more robust against spaces in the arguments. 👍 from me.

@aneeshusa
Copy link
Contributor Author

aneeshusa commented Aug 25, 2016

@bjornfor Almost all uses of cmakeFlags are using -D<name>=<value> to set CMake cache options, and those options should act as a set. If you need to specify other args, you can use the extraArgs escape hatch in cmakFlags.

@vcunat
Copy link
Member

vcunat commented Aug 25, 2016

  • Sets are nice to work with, but are you really sure that order can never play some role?
  • There will have to be a transition period where both styles are supported. In particular, I don't think it's necessary to convert many packages before there's a consensus on the style.

@aneeshusa
Copy link
Contributor Author

@vcunat This is meant to simplify the 95% case, while still making the 5% case possible. I asked on #cmake and for -D and -U, order shouldn't matter (assuming uniqueness provided by sets and no globs for -U). In cases where order does matter (using some other flags), the cmakeFlags.extraArgs can be used, or overrideDerivation can be used to bypass the mkDerivation conversion and pass whatever value you want for cmakeFlags.

In general, I would like to see build tools move towards passing more structured data (like attribute sets) between themselves, and view the form in which they are passed (environment variables, command line flags) just as a serialization detail. I think Nix and nixpkgs can help with this by making structured data more available during building (attrsets, lists, JSON), hence the PR.

Supporting both styles will be hard to do. This change already breaks overrideDerivation (since cmakeFlags becomes a single string in the derivation), necessitating overrideAttrs. Since cmakeFlags is used less frequently than say configureFlags, I would prefer to just bite the bullet and change all uses in nixpkgs, add lots of documentation, and put this change in right after a NixOS release to provide users time to update.

Also, I'd like feedback on overrideAttrs as a (better IMO) alternative to overrideDerivation.

@danbst
Copy link
Contributor

danbst commented Aug 29, 2016

Speaking of myself, I just understood that I've thought and used .overrideDerivation exactly for overriding input attributes,

I'm now confused why that worked anyway. In all my usecases .overrideAttrs was my intention. So +1

@bjornfor
Copy link
Contributor

@aneeshusa, @gebner: Ok, so a "set" is a nice abstraction for a part of the cmake interface. But isn't it confusing that a "set" attribute is named *Flags when we have many other *Flags attributes that cannot reasonably be sets? I'm thinking about configureFlags, qmakeFlags, makeFlags, sconsFlags etc. Maybe call the "set" variant cmakeCacheVariables or something?

I really like the other parts of this PR though, so I'd hate to see that list -> set conversion slow down a merge.

@gebner
Copy link
Member

gebner commented Aug 29, 2016

But isn't it confusing that a "set" attribute is named *Flags when we have many other *Flags attributes that cannot reasonably be sets?

They can already be either strings or lists depending on the *Flags. I don't see why adding another kind of *Flags increases confusion. If you grep for usage examples of cmakeFlags in nixpkgs you will see sets, and you get an error if you accidentally use a list or string.

I can live with another name, however cmakeCacheVariables seems a bit cumbersome to write.

@vcunat
Copy link
Member

vcunat commented Aug 29, 2016

cmakeFlagSet?

@aneeshusa
Copy link
Contributor Author

I think continuing to use cmakeFlags should not cause too much extra confusion, and could in fact impede discovery since you have to find the "special" name to change cmake options, so I'd like to keep using cmakeFlags.

In case we do decide to use another name, I think cmakeVars or cmakeOptions with extraFlags would be reasonable.

@edolstra
Copy link
Member

I'm not in favor of the implementation (mkCmakeFlags in stdenv/generic), as this will bloat and slow down mkDerivation even further. I'd rather add a generic mechanism to Nix to pass structured values to builders (e.g. by passing them as JSON values and maybe automatically unpacking them into bash associative arrays).

@aneeshusa
Copy link
Contributor Author

I would not want to couple Nix to a specific builder (i.e. bash). Is there a good way to do a performance benchmark to see the impact of this change before thinking about adding a new feature to Nix?

@vcunat
Copy link
Member

vcunat commented Aug 31, 2016

I don't think this could be noticeable for performance of standard nix/nixpkgs/nixos usage.

@nbp
Copy link
Member

nbp commented Sep 3, 2016

Also, I'd like feedback on overrideAttrs as a (better IMO) alternative to overrideDerivation.

This is indeed a common idiom, but I do not really see a value in adding a new abstraction for it. Also, it does not cover all cases of overrideDerivation, as it cannot be used to remove an attribute, only replacing attributes with some dummy values.

@nbp
Copy link
Member

nbp commented Sep 3, 2016

Also, overrideAttrs looks entirely unrealted to cmake, and I do not think this can be a blocker, and as such I will recommend making another pull request dedicated to this other patch, only.

@nbp nbp added the 0.kind: enhancement Add something new label Sep 3, 2016
@aneeshusa
Copy link
Contributor Author

@nbp, can you explain how overrideDerivation can be used to remove an attribute? Also, since AFAIK mkDerivation does not expose its original input attrs, I am not aware of any existing function/way/idiom to do the same thing as overrideAttrs, which is why I added it.

The reason I have put it with the cmakeFlags changes is because the cmakeFlags changes don't play well with overrideDerivation. The goal of using sets is to be able to override cmakeFlags with this kind of a function:

(oldAttrs: rec {
  cmakeFlags = oldAttrs.cmakeFlags // {
    some_cmake_flag = true;
  };
})

mkCmakeFlags renders the input cmakeFlags attrset to mkDerivation to a string in the actual derivation. Hence, when using overrideDerivation, oldAttrs.cmakeFlags will be a string, which doesn't work with //. However, overrideAttrs will pass the original cmakeFlags attrset, which works properly.

Please take a closer look at the changes I made, including the commit messages, and let me know if something is still unclear. I am happy to put overrideAttrs into a separate patch, but I wanted to include here with cmakeFlags as a motivating case study for its existence.

@danbst
Copy link
Contributor

danbst commented Sep 16, 2016

Maybe you can split the overrideAttrs to separate PR? You can then add some documentation to Nixpkgs manual, it already references override and overrideDerivation.

Also there will be clash if/when #18455 is merged.

@aneeshusa
Copy link
Contributor Author

@danbst I agree, I created #18660 for that, since it seems there have already been requests for this change.

@nbp nbp mentioned this pull request Feb 26, 2017
4 tasks
@aneeshusa
Copy link
Contributor Author

I've iterated on the design for this locally and made an RFC: NixOS/rfcs#13.
Please comment with your feedback!

There are three main parts to this change:
- Support a new `setupHookFunc` in `mkDerivation`
- Using attribute sets for `cmakeFlags` in Nix
- Passing `cmakeFlags` to the builder more smartly

== Introducing `setupHookFunc`

The `setupHook` attribute allows a derivation to register arbitrary
modifications to run in the context of downstream dependents' builds.

To enable similar modifications to be made to the attributes passed
to `mkDerivation`, teach `mkDerivation` a new trick: `setupHookFunc`.
A derivation can set `setupHookFunc` to a Nix function
which will be called for downstream dependencies
(TODO using the same logic as the setupHook inclusion),
and which takes the attribute set which was passed to `mkDerivation`
and returns a new attribute set with any desired changes.

These will stack (TODO: resolution order?) for composability,
and having these live alongside the original derivation
avoids cluttering `mkDerivation` with cmake-specific code
(which was done in an earlier iteration).

TODO: benchmark

== Using Attribute Sets for `cmakeFlags`

Almost all flags passed to `cmakeFlags` are setting CMake cache options
with "-D<name>=<value>" or "-D<name>:<type>=<value>".
Because the CMake cache is essentially a set of keys and values,
model `cmakeFlags` as an attribute set in Nix as well.

The cmake `setupHookFunc` will take a `cmakeFlags` set
and convert each key/value pair to an appropriate "-D" option for cmake.
String values are used as is, while integers are converted to strings
and booleans are converted to `"ON"` and `"OFF"`.
This presents the following benefits:
- Removal of "-D" visual noise everywhere for ease of reading.
- The `=` operater is literal, allowing extra whitespace for more ease
  of reading.
- CMake options are now first class Nix values that can be operated on
  directly with the full strength of the Nix language.
  For example, boolean values can be used directly because the
  serialization logic is consolidated into the cmake `setupHookFunc`,
  getting rid of helpers like `edf`
  and making it easier to re-use feature flags like `pythonSupport`.
- Sets automatically sort options, improving cache re-use.
- An override which updates a CMake flag option will only incur
  (amortized) O(1) cost with a set, as opposed to O(n) cost with an
  (unsorted) list, which would require a full linear search.
  It's also shorter to write: just use `//`.

A few other CMake flag types are supported:
- Setting a value to null will cause its key to be unset via "-U<name>".
- The `generator` key is mapped to the "-G<value>" option.
- The `extraArgs` key is an escape hatch; any flags in this list of
  strings are not pre-processed, but concatenated to the generated list.

Because `setupHookFunc`s are processed during `mkDerivation`,
when overriding `cmakeFlags` `overrideAttrs` should be used instead of
`overrideDerivation` so the cmake `setupHookFunc` has a chance to rerun.

== Passing `cmakeFlags` to the builder more smartly

Nix's regular conversion of lists to an environment variable
simply concatenates the contents with spaces,
which breaks if any of the flags themselves contain spaces.
`cmakeFlagsArray` was added as a way to get around this limitation,
but is unsatisfactory to use because items must be appended via bash
in a preConfigure hook.

Instead, pass the list to the builder in a smarter way:
instead of relying on Nix's conversion to a string,
perform our own conversion by escaping double quotes,
double quoting each item, and passing the flags as a Bash array,
which is hydrated via `eval`.

This handles flags with spaces, newlines and other whitespace,
as well as double quotes, faithfully.

TODO: does it handle eg some_var="$out/blah"
Make the list available during preConfigure as a bash array, so any
dynamic modifications to the CMake flags can be done there.

These changes make also `cmakeFlagsArray` redundant, so it has been
removed and replaced in all cases with `cmakeFlags`.
@aneeshusa
Copy link
Contributor Author

After seeing #44423 merged, I decided to revisit this. The main change this time is the introduction of nixSetupHook to avoid putting the mkCmakeFlags code inside mkDerivation, but instead keep it with the cmake derivation.

@@ -236,6 +243,11 @@ rec {
];
__propagatedImpureHostDeps = computedPropagatedImpureHostDeps ++ __propagatedImpureHostDeps;
};
# TODO: use dependencies/propagatedDependencies instead of just nativeBuildInputs?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ericson2314, any help you could provide here as to the correct incantation/set of dependencies to use here would be appreciated. I used nativeBuildInputs to prototype this as cmake is always in nativeBuildInputs, at least AFAIK.

@aneeshusa
Copy link
Contributor Author

aneeshusa commented Jan 9, 2019

Note to self for next update:

  • try out structured attrs mode
  • use ${placeholder "out"} and friends more to cut down on manual preConfigure changing the cmakeFlags array
  • do something similar for meson? based on a quick glance at systemd: Make build options configurable #53222

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 10, 2019

Yeah I've talked to @matthewbauer about how master...matthewbauer:meson-cross begs for this :). If you go full structured attrs, then I suppose this has featured-creaped into stdenv 2.0! I do want to land something, but I also do believe we need such a huge overhaul of stdenv, so I'm fine either way.

CC @LnL7

@LnL7
Copy link
Member

LnL7 commented Jan 10, 2019

I like the idea of using an attribute set, but I agree with the sentiment that this shouldn't happen during evaluation and probably doesn't belong in the stdenv (ie. structured attrs / stdenv2).

@Ericson2314
Copy link
Member

Oh but I think absolutely as much as possible should happen during evaluation as possible. bash is a sea of mass rebuilds and unmaintainability. We should instead fix the evaluator / nix-env -q to cope with this.

@LnL7
Copy link
Member

LnL7 commented Jan 10, 2019

I'm talking about implementation logic like this, not the values themselves. If we replace builders with functions and dynamic strings we wouldn't need structure attrs at all.

setupHookFunc = let
flattenFlag = name: value: let
serializedValue =
if builtins.isString value then value
else if builtins.isInt value then (builtins.toString value)
else if builtins.isBool value then (if value then "ON" else "OFF")
else throw "Bad value ${value} for cmakeFlags attr ${name}";
# Strings and paths are more complicated to discern,
# so just skip those
flagType = if builtins.isBool value then ":BOOL" else "";
# Unset if the value is null, otherwise set the value
in if (value == null) then "-U${name}" else"-D${name}${flagType}=${serializedValue}";
mkCmakeFlags = cmakeFlags: let
flagsList = (stdenv.lib.mapAttrsToList flattenFlag (
builtins.removeAttrs cmakeFlags [ "generator" "extraArgs" ]
)) ++ stdenv.lib.optional (cmakeFlags ? generator) "-G${cmakeFlags.generator}"
++ stdenv.lib.optionals (cmakeFlags ? extraArgs) cmakeFlags.extraArgs
;
in stdenv.lib.strings.escapeBashArray flagsList;

@mmahut
Copy link
Member

mmahut commented Aug 13, 2019

Are there any updates on this pull request, please?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/structured-cmakeflags-attrs/6261/1

@stale
Copy link

stale bot commented Sep 11, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 11, 2020
@aneeshusa
Copy link
Contributor Author

Further progress on this would require #72074.

@aneeshusa aneeshusa closed this Sep 29, 2020
@aneeshusa aneeshusa deleted the make-cmakeFlags-more-ergonomic branch September 29, 2020 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: enhancement Add something new 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: work-in-progress This PR isn't done 6.topic: stdenv Standard environment 8.has: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.