Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1556993002:
[gn] Detect location of Visual Studio in the registry. (Closed)
|
Created:
4 years, 11 months ago by Daniel Bratell Modified:
4 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[gn] Detect location of Visual Studio in the registry.
Look in the registry to figure out where Visual Studio is located
on the disk.
BUG=460462
Committed: https://crrev.com/c7af879c6c72db32930e25f20f9a9f602f11840c
Cr-Commit-Position: refs/heads/master@{#368089}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Let setup_toolchain.py use vs_toolchain.py #
Total comments: 15
Patch Set 3 : Review fixes. #
Messages
Total messages: 15 (4 generated)
bratell@opera.com changed reviewers: + brucedawson@chromium.org, scottmg@chromium.org
I know this basically adds the same 50 lines to two different files but I'm not sure how this is supposed to be organized. I could build gn with gn after this fix though.
Also, I should mention that this is a stripped down version of what gyp does.
I'm surprised this is defaulting to 2015 since we haven't switched yet. Two (matching) lines have extra indent. It is unfortunate that we're copying the code so many times. So, three copies if we include the original gyp copy? https://codereview.chromium.org/1556993002/diff/1/build/toolchain/win/setup_t... File build/toolchain/win/setup_toolchain.py (right): https://codereview.chromium.org/1556993002/diff/1/build/toolchain/win/setup_t... build/toolchain/win/setup_toolchain.py:79: return _RegistryGetValueUsingWinReg(key, value) Indented two extra spaces? https://codereview.chromium.org/1556993002/diff/1/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/1556993002/diff/1/build/vs_toolchain.py#newco... build/vs_toolchain.py:96: return _RegistryGetValueUsingWinReg(key, value) Also indented two extra spaces.
On 2016/01/04 21:36:06, brucedawson wrote: > I'm surprised this is defaulting to 2015 since we haven't switched yet. I can change that. It was just a choice that seemed more future proof. > It is unfortunate that we're copying the code so many times. So, three copies if > we include the original gyp copy? Yes, I don't want there to be two copies (I'll ignore the gyp version which is different and will eventually be obsolete) but python is not good at loading code from other scripts so you either have to make the whole thing into a library or use ugly hacks to share code. Or merge the two scripts.
I found that tools/clang/scripts/update.py already uses vs_toolchain.py so I let setup_toolchain.py do the same thing. It's still ugly but less ugly than two copies.
lgtm, but you might want to see if scottmg has any thoughts on it.
lgtm https://codereview.chromium.org/1556993002/diff/20001/build/toolchain/win/set... File build/toolchain/win/setup_toolchain.py (right): https://codereview.chromium.org/1556993002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:59: """Return path to the GYP_MSVS_VERSION of Visual Studio Trailing . on comments. https://codereview.chromium.org/1556993002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:63: this_dir = os.path.dirname(__file__) I think there's some rule about getting this before doing anything at global scope because it's affected by cwd. "SCRIPT_DIR = os.path.dirname(__file__)" or whatever after the import lines. https://codereview.chromium.org/1556993002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:81: if not 'GYP_MSVS_OVERRIDE_PATH' in os.environ: if 'GYP_MSVS_OVERRIDE_PATH' not in os.environ https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:67: elif sys.platform in ('win32', 'cygwin') and not depot_tools_win_toolchain: Did you test on cygwin? Otherwise drop that; I think it's just going to fail on _winreg anyway. (?) https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:95: try: Indent is wrong in this function. https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:102: """Return path to the GYP_MSVS_VERSION of Visual Studio nit; End with '.'. https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:114: if not version_as_year in year_to_version: "if version_as_year not in year_to_version" is more natural. https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:115: raise Exception(('Visual Studio Version %s (from GYP_MSVS_VERSION)' nit; lower case v on "version".
https://codereview.chromium.org/1556993002/diff/20001/build/toolchain/win/set... File build/toolchain/win/setup_toolchain.py (right): https://codereview.chromium.org/1556993002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:59: """Return path to the GYP_MSVS_VERSION of Visual Studio On 2016/01/05 23:30:32, scottmg (slow) wrote: > Trailing . on comments. Done. https://codereview.chromium.org/1556993002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:63: this_dir = os.path.dirname(__file__) On 2016/01/05 23:30:32, scottmg (slow) wrote: > I think there's some rule about getting this before doing anything at global > scope because it's affected by cwd. "SCRIPT_DIR = os.path.dirname(__file__)" or > whatever after the import lines. Done. https://codereview.chromium.org/1556993002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:81: if not 'GYP_MSVS_OVERRIDE_PATH' in os.environ: On 2016/01/05 23:30:32, scottmg (slow) wrote: > if 'GYP_MSVS_OVERRIDE_PATH' not in os.environ Done. https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:67: elif sys.platform in ('win32', 'cygwin') and not depot_tools_win_toolchain: On 2016/01/05 23:30:32, scottmg (slow) wrote: > Did you test on cygwin? Otherwise drop that; I think it's just going to fail on > _winreg anyway. (?) Quite likely, and yes, I did not test in cygwin. Removed cygwin from the test. https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:95: try: On 2016/01/05 23:30:32, scottmg (slow) wrote: > Indent is wrong in this function. Done. https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:114: if not version_as_year in year_to_version: On 2016/01/05 23:30:32, scottmg (slow) wrote: > "if version_as_year not in year_to_version" is more natural. Done. https://codereview.chromium.org/1556993002/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:115: raise Exception(('Visual Studio Version %s (from GYP_MSVS_VERSION)' On 2016/01/05 23:30:32, scottmg (slow) wrote: > nit; lower case v on "version". Done.
The CQ bit was checked by bratell@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/1556993002/#ps40001 (title: "Review fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1556993002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1556993002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [gn] Detect location of Visual Studio in the registry. Look in the registry to figure out where Visual Studio is located on the disk. BUG=460462 ========== to ========== [gn] Detect location of Visual Studio in the registry. Look in the registry to figure out where Visual Studio is located on the disk. BUG=460462 Committed: https://crrev.com/c7af879c6c72db32930e25f20f9a9f602f11840c Cr-Commit-Position: refs/heads/master@{#368089} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c7af879c6c72db32930e25f20f9a9f602f11840c Cr-Commit-Position: refs/heads/master@{#368089} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
