Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12298006:
Try to better guess Visual Studio path when building Dart on Windows. (Closed)
|
Created:
7 years, 10 months ago by aam-me Modified:
7 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionTry to better guess Visual Studio path.
BUG=
Committed: https://code.google.com/p/dart/source/detail?r=18736
Patch Set 1 #
Total comments: 16
Patch Set 2 : Code cleanup. #
Total comments: 8
Patch Set 3 : Looking for latest Visual Studio installed, but prefer non-Express VS2010 if it is available. #
Total comments: 2
Patch Set 4 : Got rid of arithmetic comparison with None. #Messages
Total messages: 10 (0 generated)
Hello, With this patch I'm hoping to make tools/build.py smarter so that on Windows it can detect version of Visual Studio installed. That will help all folks who are using Visual Studio 2012 or installed VS2010 on a drive other than C:\, so they don't have to specify "--devenv" option every time their build Dart. Hope this makes sense.
LGTM, but it would be great if Martin could take a look as well. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py File tools/utils.py (right): https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode67 tools/utils.py:67: if IsWindows(): if not IsWindows(): return defaultPath Then the rest doesn't need to be indented. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode80 tools/utils.py:80: "SOFTWARE\\" + wow6432Node + "Microsoft\\VisualStudio") Long line. Also, I think our Python style guide prefer using % rather than string concatenation with +. So this should be: "SOFTWARE\\%sMicrosoft\\VisualStudio" % wow6432Node https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode81 tools/utils.py:81: except WindowsError: Please document why it is OK to ignore this error. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode84 tools/utils.py:84: iSubkey = 0 What does this variable mean? The word "iSubkey" is a bit strange. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode85 tools/utils.py:85: while(True): I don't think you need parentheses here in Python. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode89 tools/utils.py:89: # Reached end of enumeration Is there a way to check, or must you catch an exception? https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode92 tools/utils.py:92: if not match is None: if match: https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode102 tools/utils.py:102: _winreg.CloseKey(key) This is dead code as it comes after an "infinite" loop.
Thank you, Peter! I appreciate the comments very much. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py File tools/utils.py (right): https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode67 tools/utils.py:67: if IsWindows(): On 2013/02/18 15:47:20, ahe wrote: > if not IsWindows(): > return defaultPath > > Then the rest doesn't need to be indented. Done. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode80 tools/utils.py:80: "SOFTWARE\\" + wow6432Node + "Microsoft\\VisualStudio") On 2013/02/18 15:47:20, ahe wrote: > Long line. Also, I think our Python style guide prefer using % rather than > string concatenation with +. So this should be: > > "SOFTWARE\\%sMicrosoft\\VisualStudio" % wow6432Node Done. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode81 tools/utils.py:81: except WindowsError: On 2013/02/18 15:47:20, ahe wrote: > Please document why it is OK to ignore this error. Done. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode84 tools/utils.py:84: iSubkey = 0 This is subkey counter. Changed name. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode85 tools/utils.py:85: while(True): On 2013/02/18 15:47:20, ahe wrote: > I don't think you need parentheses here in Python. Done. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode89 tools/utils.py:89: # Reached end of enumeration According to http://docs.python.org/2/library/_winreg.html#_winreg.EnumKey I have to catch exception, unfortunately. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode92 tools/utils.py:92: if not match is None: On 2013/02/18 15:47:20, ahe wrote: > if match: Done. https://chromiumcodereview.appspot.com/12298006/diff/1/tools/utils.py#newcode102 tools/utils.py:102: _winreg.CloseKey(key) Thanks. Wrapped it into try: finally: to close the key.
https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/build.py File tools/build.py (right): https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/build.py#newc... tools/build.py:48: 'different versions of Visual Studio)', default='devenv.com') The '--executable' options says that the name of the executable varies for different versions of visual studio. I have no idea if this is true. But if it is true, then '--devenv' should probably be kept in sync with '--executable' (which means that if 'utils.GuessVisualStudioPath()' picks visual studio version X, we need the executable name corresponding to VS version X as well). To do this, you could change 'utils.GuessVisualStudioPath()' to return a tuple like this: def BuildOptions(): (vs_directory, vs_executable) = utils.GuessVisualStudioPath() result = optparse.OptionParser() .... result.add_option('--devenv', default=vs_directory, ...) result.add_option('--executable', default=vs_executable, ...) https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/utils.py File tools/utils.py (right): https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/utils.py#newc... tools/utils.py:65: def GuessVisualStudioPath(): I have one major concern with this function: - In the previous version we've always used Visual Studio 10. - GuessVisualStudioPath picks the first best Visual Studio version it finds in the registry (not necessarily the newest version), falling back to 10 if it can't find any version. - We have currently up to 3 different versions of Visual Studio installed on the BuildBots (AFAIK). Now this can result in a situation where we pick different versions of VS on different bots. This, in return, can result in hard-to-find building issues, flakiness, ... on the bots (we had already issues with gyp and different VS versions). Here's what I would prefer: If VS 10 is installed, just take it (so we don't change the default behavior of build.py). If VS 10 is not installed, do the registry lookup and take any version: def GuessVisualStudioPath(): defaultPath = r'C:\Program Files ...' if os.path.exists(defaultPath): return defaultPath else: import win32process .... https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/utils.py#newc... tools/utils.py:67: '\\IDE' In general I usually prefer raw strings for windows path names (to eliminate double backslashes). https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/utils.py#newc... tools/utils.py:96: match = re.match('^(\\d+\\.\\d+)$', subkeyName) Consider using a raw string.
> Here's what I would prefer: If VS 10 is installed, just take it (so we don't > change the default behavior of build.py). If VS 10 is not installed, do the > registry lookup and take any version: Actually, it would be even better if we: - search the registry for all installed visual studios (collecting the corresponding VS InstallPath's in a list of (version, path)-tuples [or use dictionary] ) - see if we have 10.0 installed, if so take it - otherwise take the newest version
Thank you, Martin, for the review! I understand what you are saying and uploaded new patch with the changes. Please, take another look when you have a chance.
Hi, Martin, here are my responses to your comments :-) Thanks, Alexander Aprelev. https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/build.py File tools/build.py (right): https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/build.py#newc... tools/build.py:48: 'different versions of Visual Studio)', default='devenv.com') On 2013/02/18 21:17:52, kustermann wrote: > The '--executable' options says that the name of the executable varies for > different versions of visual studio. > I have no idea if this is true. But if it is true, then '--devenv' should > probably be kept in sync with '--executable' (which means that if > 'utils.GuessVisualStudioPath()' picks visual studio version X, we need the > executable name corresponding to VS version X as well). > > To do this, you could change 'utils.GuessVisualStudioPath()' to return a tuple > like this: > > def BuildOptions(): > (vs_directory, vs_executable) = utils.GuessVisualStudioPath() > result = optparse.OptionParser() > .... > result.add_option('--devenv', default=vs_directory, ...) > result.add_option('--executable', default=vs_executable, ...) Done. https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/utils.py File tools/utils.py (right): https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/utils.py#newc... tools/utils.py:65: def GuessVisualStudioPath(): Done. This script can also retrieve Visual Studio version it should use from some environment variable(BUILD_DART_MSVS_VERSION, for example, similar to GYP_MSVS_VERSION) that would override whatever guessing logic is in the script. That might help with flakiness caused by different builder setup. https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/utils.py#newc... tools/utils.py:67: '\\IDE' On 2013/02/18 21:17:52, kustermann wrote: > In general I usually prefer raw strings for windows path names (to eliminate > double backslashes). Done. https://chromiumcodereview.appspot.com/12298006/diff/7003/tools/utils.py#newc... tools/utils.py:96: match = re.match('^(\\d+\\.\\d+)$', subkeyName) On 2013/02/18 21:17:52, kustermann wrote: > Consider using a raw string. Done.
LGTM. Thank you for making the life of dart developers less painful on windows ;-). https://chromiumcodereview.appspot.com/12298006/diff/5003/tools/utils.py File tools/utils.py (right): https://chromiumcodereview.appspot.com/12298006/diff/5003/tools/utils.py#newc... tools/utils.py:129: if version > bestGuess[0]: The first time we hit this comparision, we compare 'version' with None. Although this works in python versions 2.X, it is not optimal (it will raise an exception in python3 AFAIK). You can keep it, but you could also do bestGuess = (0.0, (defaultPath, defaultExecutable)) on line 91.
Thank you, Martin! https://chromiumcodereview.appspot.com/12298006/diff/5003/tools/utils.py File tools/utils.py (right): https://chromiumcodereview.appspot.com/12298006/diff/5003/tools/utils.py#newc... tools/utils.py:129: if version > bestGuess[0]: Done. Thanks, didn't know that.
Message was sent while issue was closed.
Committed patchset #4 manually as r18736 (presubmit successful). |
