-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Support envs on external render commands #5278
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
Support envs on external render commands #5278
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5278 +/- ##
==========================================
- Coverage 37.42% 37.42% -0.01%
==========================================
Files 312 312
Lines 46480 46480
==========================================
- Hits 17394 17393 -1
Misses 26596 26596
- Partials 2490 2491 +1
Continue to review full report at Codecov.
|
Hmm... I'm not sure there should be differential expansion based on whether the machine is running windows or otherwise. I guess we only want (In passing I'm also quite suspicious of the use of |
@zeripath see golang/go#24848, |
It's not that os.Expand doesn't support Windows, but that it won't support Windows-style variables. Currently we're not doing any expansion of variables, so the choice is to support different variable expansion regimes for Windows vs. others or just to do the same. Hmm... Arguments in favour of 'doze expansion:
Arguments against specific 'doze expansion:
I'm sorry, I think you're right we should do 'doze expansion. (I've just checked go's documentation, is my interpretation right in that the filepath for the command would have to be in 'doze format with backslashes? e.g. "C:\Program Files (x86)\Why so many spaces\command.exe" - if not that probably needs looking at too.) It might still be worth handling |
@zeripath currently just these two and I don't think we need to support |
An improvement for #5201. @Cellebyte