Add "environment" as a special attribute

[sgraham]

Oops, forgot to click pull.

This optionally sets the environment block for Windows subprocesses. Do you think this is a reasonable approach to getting rid of the wrapper on win?

[buildhive]

Evan Martin » ninja #427 SUCCESS
This pull request looks good
(what's this?)

[evmar]

Random idea: what if you could just provide the environment block directly to ninja as a value?
That is, rather than writing out an environment file at gyp time then telling ninja where to find the file, instead gather the environment data as a string and pass it through to ninja as a variable.

e.g.
n.variable('env64', msvs_emulation._FormatAsEnvironmentBlock(...))
and then later
n.build(..., environment='$env64')

That would avoid all the path writing and loading code.

Thinking aloud: it could cost more in terms of ninja doing more string wrangling.

[sgraham]

Hmm, it'd have to be encoded somehow (it's \0 terminated strings in a \0 terminated list). I guess it could be base64d or something but I'm not sure that's worth avoiding the file code for.

Or I guess some other text-friendly placeholder for \0 that could be substituted at an appropriate time.

[sgraham]

(Oh, goma hacks in there replaces the files with its own... not that that's ninja's problem of course. Perhaps good motivation to stop doing that.)

[evmar]

We already have escaping, so we could just encode nul as $0.

brevity due to phone
On Apr 29, 2013 4:45 PM, "Scott Graham" notifications@github.com wrote:

Hmm, it'd have to be encoded somehow (it's \0 terminated strings in a \0
terminated list). I guess it could be base64d or something but I'm not sure
that's worth avoiding the file code for.

Or I guess some other text-friendly placeholder for \0 that could be
substituted at an appropriate time.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/561#issuecomment-17201821
.

[sgraham]

OK, I'll write that up.

On Mon, Apr 29, 2013 at 5:06 PM, Evan Martin notifications@github.comwrote:

We already have escaping, so we could just encode nul as $0.

brevity due to phone
On Apr 29, 2013 4:45 PM, "Scott Graham" notifications@github.com wrote:

Hmm, it'd have to be encoded somehow (it's \0 terminated strings in a \0
terminated list). I guess it could be base64d or something but I'm not
sure
that's worth avoiding the file code for.

Or I guess some other text-friendly placeholder for \0 that could be
substituted at an appropriate time.

—
Reply to this email directly or view it on GitHub<
https://github.com/martine/ninja/pull/561#issuecomment-17201821>
.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/561#issuecomment-17202548
.

[evmar]

If you want any help, I'm happy to do the $0 expansion bit as it's orthogonal. (You can also tell me if you think it's a dumb idea to inline these strings into ninja)

[sgraham]

Sorry, Chrome Win hasn't been linking since Friday, hair on fire, blah blah.

Yes, please feel free to do the $0 part, it's slightly annoying to do on Windows because re2c always emits \r\n.

I'm pretty ambivalent on whether strings or files is better. Files is easier for me because that's what gyp already knows how to do. Values seems a bit tidier, but a variety of other special attributes are currently file names too, so ... shrug.

[evmar]

https://gist.github.com/martine/5577207 (patch for $0)

[nico]

I always said that I thought just setting the env via e.g. ninja -t msvc -e would be fine, but iirc @ngladitz collected data for me that shows that the subprocess overhead from that is actually measurable. (I think that data is still in a tab on another computer, but I can't find it now.) So this might be something we might want to do in the future, but it's a bit messy and the win wasn't that huge iirc. But let's keep this open for now.

[nico]

Here's the link ngladitz sent me: https://github.com/ngladitz/cmake/commits/ninja-cl-deps-performance

[evmar]

I suspect if we went down the route of the inline environment block we'd run into requests for:

• a way to express just additions/removals to the outer environment
• a more convenient way to express them than as a long block of foo$0bar$0baz$0$0
  Perhaps we can punt both of those to just say it's the generator's problem, though.

This is a longer way of saying: it'd be nice to collect all the requirements of users before we commit to any new feature.

[jhasse]

Closing due to merge conflicts. If you still think this is relevant, feel free to reopen :)