WIP/RFC: stdenv: deprecate passing a string for configureFlags

[aneeshusa]

Motivation for this change

See below.

Things done

• Tested using sandboxing
  (nix.useSandbox on NixOS,
  or option build-use-sandbox 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.

---

Currently, configureFlags can be passed as a string or a list.
Passing a string works OK for small cases but it becomes hard to manage
interspersing spaces between the flags with many options, and doesn't
match the semantics of each flag being a separate item.
Passing a list (of strings) is a better fit for the type of data, but
it currently relies on Nix's regular conversion to an environment
variable, which simply concatenates the contents with spaces.
This breaks if any of the configure flags themselves contain spaces.
configureFlagsArray 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.

So, update configureFlags to be more ergonomic:

• Deprecate passing a string for configureFlags (require a list),
  and include helpful trace/abort messages.
• 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
  interspersing tabs. Tabs should be fairly rare in configureFlags;
  in case a literal tab needs to be passed, it must be given extra
  escapes:
  configureFlags = [ "--with-some-arg-with-a-\t-character" ](The only alternative would require usage of eval.)
  This also fixes passing flags that contain spaces.
• Make the list available during preConfigure as a bash array, so any
  dynamic modifications to the configure flags can be done there.

There are roughly 1500 uses of configureFlags at present, which is too
many to change all at one time, and it is a commonly used flag (e.g.
in packages outside of nixpkgs). So, use a generous deprecation
timetable: start warning in the next release (16.09), and convert to a
hard error in the release after that (17.03). Since we are mid-way
between releases, this gives 9 months and 1.5 releases for users to
upgrade; switching from strings to lists is not too hard.

These changes make also configureFlagsArray redundant. There are roughly
70 uses of configureFlagsArray at present, which is enough to remove all
at once (in a future patchset).

This is a WIP that I'm posting as an RFC; I've updated enough files
to be able to build bash successfully. More to come if this is
welcomed, but I'd like to get help with converting all the uses.
I'd also like to know which docs need to be updated.

[mention-bot]

By analyzing the blame information on this pull request, we identified @shlevy, @edolstra and @garbas to be potential reviewers

[domenkozar]

It's currently very hard to do deprecation warnings in Nix (we don't have a good way), for example those will show up in nix-env -qa. I advise an issue is opened like #4491 and once we've removed them, we can prohibit the string.

[domenkozar]

See discussion in #15357

[vcunat]

The general idea

When I think of it, most of list-type mkDerivation parameters would be simpler as bash arrays in the builder, and we've converted a few of them already. For a way of passing nix lists into bash arrays, both \t and \n seem significantly more reliable than spaces, so either is a good start IMHO and could improve later (\1..\31 range is likely to be most useful).

(The attrsets suggested in another PR would best be converted to a list of strings in the nix language already.)

Transition period

To avoid necessity of converting every single package at once, mkDerivation could check whether it's a list and replace spaces by tabs directly if it's a plain string. Still, any part of this shouldn't be merged before 16.09 is branched. As discussed elsewhere, warnings thrown in this way tend to reach unintended audience in most cases (users that just want some package installed).

[aneeshusa]

@vcunat to be clear - in the other PR (#17886), mkDerivation converts attrset -> list of strings (cmakeFlags.extraArgs is appended here) -> single tab-separated string. Not sure if that's what you mean by "converted ... in the nix language already".

No problem with waiting for 16.09 branch-off. This PR needs to be updated (e.g. ad automatic tab escaping), and I've realized after #17886 this approach works much better with overrideAttrs (introduced in that PR) instead of overrideDerivation, which will be a big docs change.

I agree that throwing warnings at end-users is not the most helpful, but there should be some way of notifying maintainers of these packages to update them. In this case since both strings and lists are supported now we could ask maintainers to start changing configureFlags to lists now, and then this PR will just disallow strings later (and use the tab separation).

[edolstra]

Passing an array of strings is equivalent to passing a concatenated string interspersed with spaces. So deprecating strings doesn't really accomplish anything except a huge amount of code churn. The reason for configureFlagsArray is that you cannot include whitespace in values, i.e. configureFlags = [ "foo bar"] is equivalent to configureFlags = "foo bar" (so passes two arguments). Enforcing configureFlags to be an array may give the false impression that it's "safe" to pass arrays via derivations attributes. (Hackery like using tabs as separators isn't really a solution IMHO.)

However, it would be great if Nix had a way to pass real arrays (and even simple attribute sets) to builders. But that would require some not entirely trivial changes (e.g. the .drv file format would have to support arrays in environment variables)

[vcunat]

@edolstra: you would have to read more carefully, as the point of this PR is to side-step the default way of how lists are passed to the builder (and it uses tabs instead of spaces ATM).

[Ericson2314]

It's possible the number of non-list uses of configureFlags is pretty low? I'd be OK with just hard-removing support for that. Since I added configurePlatforms support, it's actually quite to make this an error as configureFlags gets appended anyways.

[dtzWill]

I like this a lot, what's left to discuss/fix?

[Ericson2314]

I think if we rebase a lot of this will shrink dramatically. That's what I remember from my configurePlatforms implementation.

[aneeshusa]

Closing in favor of #45886.