Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1805173002:
win toolchain: Load env from json file, not .cmd file. (Closed)
Descriptionwin toolchain: Load env from json file, not .cmd file.
No behavior change. The motivation is that loading the json file works
on non-Windows too.
Requires a toolchain that was built with
https://codereview.chromium.org/1706423002 and
https://codereview.chromium.org/1776283002/ applied
(such as the current 2015 package).
BUG=495204
Committed: https://crrev.com/d510820d669ae1c9f1fc1c15a76ccc6dfb1fa83c
Cr-Commit-Position: refs/heads/master@{#381398}
Patch Set 1 #
Total comments: 7
Patch Set 2 : comments #Messages
Total messages: 19 (8 generated)
Description was changed from ========== win toolchain: Load env from json file, not .Cmd file. No behavior change. The motivation is that loading the json file works on non-Windows too. BUG=495204 ========== to ========== win toolchain: Load env from json file, not .Cmd file. No behavior change. The motivation is that loading the json file works on non-Windows too. Requires a toolchain that was built with https://codereview.chromium.org/1706423002 and https://codereview.chromium.org/1776283002/ applied (such as the current 2015 package). BUG=495204 ==========
thakis@chromium.org changed reviewers: + scottmg@chromium.org
lgtm https://codereview.chromium.org/1805173002/diff/1/build/toolchain/win/setup_t... File build/toolchain/win/setup_toolchain.py (right): https://codereview.chromium.org/1805173002/diff/1/build/toolchain/win/setup_t... build/toolchain/win/setup_toolchain.py:95: entries = [os.path.join(*([os.path.join(sdk_dir, 'bin') ] + e)) extra space after 'bin') https://codereview.chromium.org/1805173002/diff/1/build/toolchain/win/setup_t... build/toolchain/win/setup_toolchain.py:102: if k not in env: env[k] = os.environ[k] \n after : https://codereview.chromium.org/1805173002/diff/1/build/toolchain/win/setup_t... build/toolchain/win/setup_toolchain.py:109: # Check that the json file contained the same environment as the .Cmd file. No capital C on .cmd here and next line. Do you want if not sys.platform(win) here too? https://codereview.chromium.org/1805173002/diff/1/build/toolchain/win/setup_t... build/toolchain/win/setup_toolchain.py:111: assert _ExtractImportantEnvironment(variables) == \ Good idea!
Description was changed from ========== win toolchain: Load env from json file, not .Cmd file. No behavior change. The motivation is that loading the json file works on non-Windows too. Requires a toolchain that was built with https://codereview.chromium.org/1706423002 and https://codereview.chromium.org/1776283002/ applied (such as the current 2015 package). BUG=495204 ========== to ========== win toolchain: Load env from json file, not .cmd file. No behavior change. The motivation is that loading the json file works on non-Windows too. Requires a toolchain that was built with https://codereview.chromium.org/1706423002 and https://codereview.chromium.org/1776283002/ applied (such as the current 2015 package). BUG=495204 ==========
thanks! https://codereview.chromium.org/1805173002/diff/1/build/toolchain/win/setup_t... File build/toolchain/win/setup_toolchain.py (right): https://codereview.chromium.org/1805173002/diff/1/build/toolchain/win/setup_t... build/toolchain/win/setup_toolchain.py:95: entries = [os.path.join(*([os.path.join(sdk_dir, 'bin') ] + e)) On 2016/03/15 22:58:54, scottmg wrote: > extra space after 'bin') Done. https://codereview.chromium.org/1805173002/diff/1/build/toolchain/win/setup_t... build/toolchain/win/setup_toolchain.py:102: if k not in env: env[k] = os.environ[k] On 2016/03/15 22:58:54, scottmg wrote: > \n after : Done. https://codereview.chromium.org/1805173002/diff/1/build/toolchain/win/setup_t... build/toolchain/win/setup_toolchain.py:109: # Check that the json file contained the same environment as the .Cmd file. On 2016/03/15 22:58:54, scottmg wrote: > No capital C on .cmd here and next line. Do you want if not sys.platform(win) > here too? Done. This doesn't have any sys.platform checks yet, I figured I'd add them in one fell swoop later.
The CQ bit was checked by thakis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1805173002/#ps20001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805173002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805173002/20001
Message was sent while issue was closed.
Description was changed from ========== win toolchain: Load env from json file, not .cmd file. No behavior change. The motivation is that loading the json file works on non-Windows too. Requires a toolchain that was built with https://codereview.chromium.org/1706423002 and https://codereview.chromium.org/1776283002/ applied (such as the current 2015 package). BUG=495204 ========== to ========== win toolchain: Load env from json file, not .cmd file. No behavior change. The motivation is that loading the json file works on non-Windows too. Requires a toolchain that was built with https://codereview.chromium.org/1706423002 and https://codereview.chromium.org/1776283002/ applied (such as the current 2015 package). BUG=495204 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== win toolchain: Load env from json file, not .cmd file. No behavior change. The motivation is that loading the json file works on non-Windows too. Requires a toolchain that was built with https://codereview.chromium.org/1706423002 and https://codereview.chromium.org/1776283002/ applied (such as the current 2015 package). BUG=495204 ========== to ========== win toolchain: Load env from json file, not .cmd file. No behavior change. The motivation is that loading the json file works on non-Windows too. Requires a toolchain that was built with https://codereview.chromium.org/1706423002 and https://codereview.chromium.org/1776283002/ applied (such as the current 2015 package). BUG=495204 Committed: https://crrev.com/d510820d669ae1c9f1fc1c15a76ccc6dfb1fa83c Cr-Commit-Position: refs/heads/master@{#381398} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d510820d669ae1c9f1fc1c15a76ccc6dfb1fa83c Cr-Commit-Position: refs/heads/master@{#381398}
Message was sent while issue was closed.
On 2016/03/16 05:28:46, commit-bot: I haz the power wrote: > Patchset 2 (id:??) landed as > https://crrev.com/d510820d669ae1c9f1fc1c15a76ccc6dfb1fa83c > Cr-Commit-Position: refs/heads/master@{#381398} Because VS 2013 has not been rebuilt with the CLs listed in the description this change means that VS 2013 gn builds no longer work. Details in the bug.
Message was sent while issue was closed.
bradnelson@google.com changed reviewers: + bradnelson@google.com
Message was sent while issue was closed.
This is horking our sdk release bots: https://uberchromegw.corp.google.com/i/client.nacl.sdk/builders/windows-sdk-m... I'm gonna send a change to make it fallback to the old behavior, for now.
Message was sent while issue was closed.
That means you're using a much older toolchain than chromium. Can you roll forward your toolchain?
Message was sent while issue was closed.
On 2016/04/08 20:37:29, Nico wrote: > That means you're using a much older toolchain than chromium. Can you roll > forward your toolchain? I can package up a 2013 toolchain with this file embedded, but we should be moving to 2015 - we'll probably require it soon.
Message was sent while issue was closed.
On 2016/04/11 17:03:42, brucedawson (out until 11th) wrote: > On 2016/04/08 20:37:29, Nico wrote: > > That means you're using a much older toolchain than chromium. Can you roll > > forward your toolchain? > > I can package up a 2013 toolchain with this file embedded, but we should be > moving to 2015 - we'll probably require it soon. If that's not too hard, that'd be cool. Even if Chromium requires 2015, nacl can choose to still support 2013 in their code. |
