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

doc: specify that *Flags are string lists #9607

Merged
merged 1 commit into from
Aug 31, 2016
Merged

doc: specify that *Flags are string lists #9607

merged 1 commit into from
Aug 31, 2016

Conversation

nckx
Copy link
Member

@nckx nckx commented Sep 2, 2015

Current practice is wildly inconsistent:

nixpkgs $ egrep -r '(configure|build|install|make)Flags = \[' | wc -l
935
nixpkgs $ egrep -r '(configure|build|install|make)Flags = "' | wc -l
687

@nckx nckx added 0.kind: enhancement Add something new documentation 9.needs: reporter feedback This issue needs the person who filed it to respond labels Sep 2, 2015
@vcunat vcunat changed the title doc: specify that *Flags are string arrays doc: specify that *Flags are string lists Sep 2, 2015
@vcunat
Copy link
Member

vcunat commented Sep 2, 2015

I certainly agree, but I want to leave some timeframe for others to react.

@vcunat vcunat added this to the 15.09 milestone Sep 2, 2015
@nckx
Copy link
Member Author

nckx commented Sep 2, 2015

Thanks. (D'oh. Nice catch. I changed the title of the commit as well.)

@vcunat
Copy link
Member

vcunat commented Sep 2, 2015

We call them lists, but unlike what's typical in functional languages, they actually are implemented as contiguous arrays IIRC.

@abbradar
Copy link
Member

abbradar commented Sep 3, 2015

+1, I've been fixing occurrences of plain strings in makeFlags where I found them for quite some time.

@nckx
Copy link
Member Author

nckx commented Sep 3, 2015

There was a time when I wrote them as a single string myself, because that makes it harder to forget that spaces aren't allowed. TBH I still think that's a weird implementation issue that needs to be fixed, but assume there are good reasons for not just throwing everything at the shell quoted.

@lucabrunox
Copy link
Contributor

If you want to say it's a list of string, I think it must be noted that it's just a string in the end in the builder. The fact is that all *Flags really are strings, but nix converts lists to string. So I don't think that description is really good.

In theory the correct thing is writing *Flags as strings, but because we like lists, we use lists and nix does the rest.

@nckx
Copy link
Member Author

nckx commented Sep 3, 2015

@lethalman:

we like lists

Lists are rad - when they're not easily (and worse, quietly) broken by spaces. The current situation seems like the worst of both worlds.

it's just a string in the end in the builder

Honestly, if I was sure there was no hope of that changing and thought a PR changing *Flags to strings instead would stand a chance, I would have submitted that instead. But people (I remember not who) seemed less than wild about that idea.

I just wanted to hear some consensus on this before I started asking silly questions like: why aren't lists exported as bash arrays, besides the obvious compatibility reasons?

@lucabrunox
Copy link
Contributor

It's just not true that it's a list, so I find this PR misleading. The truth in the docs should be: it's a string, but you can pass a list of strings and nix will automatically join it with spaces because of builtins.toString.

@abbradar
Copy link
Member

abbradar commented Sep 3, 2015

To elaborate my opinion: I feel that logically *Flags have "list of string flags" type, so our first effort should be spent at abstracting them as "lists of string flags", and only if that effort is too big/complicated we may decide to do otherwise. That effort includes promoting usage of correct types (while NixOS/nix#14 is not yet there ^_^), although explanation of caveats might be needed -- so a better way may be adding "Internally it's converted into a string separated with spaces" note. Also that effort may include (in some future) representing these lists as Bash (or other shell) arrays -- sounds good (if we don't talk about compatibility), but that's out of range of this PR.

@vcunat
Copy link
Member

vcunat commented Sep 3, 2015

I believe we agree that we would like to truly have the flags as lists of strings in future. Why not have that formulation now, to announce that intention, but we should add a note that spaces in these strings aren't supported (yet). Then we can improve the implementation later.

(Out of scope of this PR.) Note that our stdenv does accept e.g. configureFlagsArray already, treated as a bash array. I only don't know of a comfortable way of defining it, and I directly can't see how to pass arrays without modifications on the nix level. Actually, all nix arrays might be just passed as bash arrays into the builder, but I imagine we would need to have a switch somewhere in the derive primitive to maintain backward compatibility.

@nckx
Copy link
Member Author

nckx commented Sep 4, 2015

I fully agree with @vcunat's last comment and will add that caveat to the PR.

@domenkozar
Copy link
Member

ping @nckx :)

@fpletz fpletz modified the milestones: 16.09, 15.09 Jun 26, 2016
@domenkozar domenkozar merged commit 7b3a361 into NixOS:master Aug 31, 2016
@vcunat
Copy link
Member

vcunat commented Aug 31, 2016

There's a better approach: #15799 EDIT: I didn't notice you merged :-) but it'll be good after that issue is solved (implemented and merged).

@aneeshusa
Copy link
Contributor

If *Flags are only for string lists, please chime in on #17886 about ideas for another name for cmakeFlags (that PR turns it from a list to a set).

@Artturin Artturin added 6.topic: documentation Meta-discussion about documentation and its workflow and removed old-label: documentation labels Apr 19, 2023
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 6.topic: documentation Meta-discussion about documentation and its workflow 9.needs: reporter feedback This issue needs the person who filed it to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants