From a245db4b2d5183ee27e1d13a9db454ce694d94d2 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Tue, 8 Mar 2022 10:02:42 -0700 Subject: [PATCH 1/7] Tweak the MSVC environment vars cache - Now performs a sanity check: if the retrieved tools path does not exist, consider the entry invalid so it will be recomputed. - The dictionary key, which is the name of a batch file, is computed a bit differently: the dashes are left off if there are no arguments. - The cachefile is changed to have a .json suffix, for better recognition on Windows systems. Signed-off-by: Mats Wichmann --- CHANGES.txt | 6 ++++++ RELEASE.txt | 3 +++ SCons/Tool/MSCommon/common.py | 2 +- SCons/Tool/MSCommon/vc.py | 15 ++++++++++++++- doc/man/scons.xml | 2 +- 5 files changed, 25 insertions(+), 3 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index d36e91130d..e7be169311 100755 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -56,6 +56,12 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER processors for testing threads. - Fixed crash in C scanner's dictify_CPPDEFINES() function which happens if AppendUnique is called on CPPPATH. (Issue #4108). + - The MSVC script_env_cache now contains a sanity check: if the retrieved + tools path does not exist, it is invalidated so it will be recomputed. + The dictionary key, which is the name of a batch file, is also computed + a bit differently: the dashes are left off if there are no arguments. + The cachefile is changed to have a .json suffix, for better recognition + on Windows where there might, for example, be an editor association. From Zhichang Yu: - Added MSVC_USE_SCRIPT_ARGS variable to pass arguments to MSVC_USE_SCRIPT. diff --git a/RELEASE.txt b/RELEASE.txt index 4f963b5149..80b8bf1c6d 100755 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -36,6 +36,9 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY - The change to "content" and "content-timestamp" Decider names is reflected in the User Guide as well, since the hash function may be other than md5 (tidying up from earlier change) +- The SCONS_CACHE_MSVC_CONFIG behavior changes slightly: will now do a + sanity check to hopefully regenerate if the compiler version has changed, + rather than fail; the default filename now has a .json suffix. FIXES diff --git a/SCons/Tool/MSCommon/common.py b/SCons/Tool/MSCommon/common.py index ee31b2dd8b..fee460a9a6 100644 --- a/SCons/Tool/MSCommon/common.py +++ b/SCons/Tool/MSCommon/common.py @@ -94,7 +94,7 @@ def debug(x, *args): # SCONS_CACHE_MSVC_CONFIG is public, and is documented. CONFIG_CACHE = os.environ.get('SCONS_CACHE_MSVC_CONFIG') if CONFIG_CACHE in ('1', 'true', 'True'): - CONFIG_CACHE = os.path.join(os.path.expanduser('~'), '.scons_msvc_cache') + CONFIG_CACHE = os.path.join(os.path.expanduser('~'), '.scons_msvc_cache.json') def read_script_env_cache(): diff --git a/SCons/Tool/MSCommon/vc.py b/SCons/Tool/MSCommon/vc.py index fe31cb3f08..36a530e996 100644 --- a/SCons/Tool/MSCommon/vc.py +++ b/SCons/Tool/MSCommon/vc.py @@ -43,6 +43,7 @@ import subprocess import os import platform +from pathlib import Path from string import digits as string_digits from subprocess import PIPE @@ -741,8 +742,20 @@ def script_env(script, args=None): if script_env_cache is None: script_env_cache = common.read_script_env_cache() - cache_key = "{}--{}".format(script, args) + extra = f"--{args}" if args else "" + cache_key = f"{script}{extra}" cache_data = script_env_cache.get(cache_key, None) + + # brief sanity check + if cache_data is not None: + try: + cache_check = cache_data["VCToolsInstallDir"][0] + toolsdir = Path(cache_check) + if not toolsdir.exists(): + cache_data = None + except (KeyError, IndexError): + cache_data = None + if cache_data is None: stdout = common.get_output(script, args) diff --git a/doc/man/scons.xml b/doc/man/scons.xml index 047f230e18..d7fe39ddf1 100644 --- a/doc/man/scons.xml +++ b/doc/man/scons.xml @@ -8082,7 +8082,7 @@ in tightly controlled Continuous Integration setups. If set to a True-like value ("1", "true" or "True") will cache to a file named -.scons_msvc_cache in the user's home directory. +.scons_msvc_cache.json in the user's home directory. If set to a pathname, will use that pathname for the cache. Note: use this cache with caution as it From 50ddff517f22b34b8d2054c4bcbb616a9efb0eb6 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Fri, 11 Mar 2022 08:47:31 -0700 Subject: [PATCH 2/7] MSVS_CACHE: Reword the CHANGES and RELEASE entries Sharpened up the descriptions of the change in PR 4111. Changed the opening of the cache file to use pathlib functions. Signed-off-by: Mats Wichmann --- CHANGES.txt | 13 ++++++++----- RELEASE.txt | 20 ++++++++++++-------- SCons/Tool/MSCommon/common.py | 9 ++++++--- 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index e7be169311..41b02d97b1 100755 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -57,11 +57,14 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER - Fixed crash in C scanner's dictify_CPPDEFINES() function which happens if AppendUnique is called on CPPPATH. (Issue #4108). - The MSVC script_env_cache now contains a sanity check: if the retrieved - tools path does not exist, it is invalidated so it will be recomputed. - The dictionary key, which is the name of a batch file, is also computed - a bit differently: the dashes are left off if there are no arguments. - The cachefile is changed to have a .json suffix, for better recognition - on Windows where there might, for example, be an editor association. + tools path does not exist, the entry is invalidated so it will + be recomputed, in an attempt to avoid scons failing when certain + compiler version bumps have taken place. The dictionary key (uses + the name of a batch file and any arguments which may have been + passes), is now computed a bit differently: the dashes are left + off if there are no arguments. The default cachefile is changed + to have a .json suffix, for better recognition on Windows since + the contents are json. From Zhichang Yu: - Added MSVC_USE_SCRIPT_ARGS variable to pass arguments to MSVC_USE_SCRIPT. diff --git a/RELEASE.txt b/RELEASE.txt index 80b8bf1c6d..c021a404a7 100755 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -26,19 +26,23 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY --------------------------------------- - On Windows, %AllUsersProfile%\scons\site_scons is now the default "system" - location for a site_scons. %AllUsersProfile%\Application Data\scons\site_scons - will continue to work. There does not seem to be any convention to use - an "Application Data" subdirectory here. + location for a site_scons directory. + %AllUsersProfile%\Application Data\scons\site_scons will continue to work. + There does not seem to be any existing convention to use an + "Application Data" subdirectory here. - Action._subproc() can now be used as a python context manager to ensure that the POpen object is properly closed. -- SCons help (-H) no longer prints the "ignored for compatibility" options, - which are still listed in the manpage. +- SCons help (-H) no longer prints the "ignored for compatibility" options + to save some space (these are still listed in the manpage). - The change to "content" and "content-timestamp" Decider names is reflected in the User Guide as well, since the hash function may be other than md5 (tidying up from earlier change) -- The SCONS_CACHE_MSVC_CONFIG behavior changes slightly: will now do a - sanity check to hopefully regenerate if the compiler version has changed, - rather than fail; the default filename now has a .json suffix. +- If SCONS_CACHE_MSVC_CONFIG is used, it will now attempt a sanity check for + the cached compiler information, and regenerate the information + if needed, rather than just failing after certain compiler version + changes have happened. The cache file can still be manually removed + if there are issues to force a regen. The default cache filename now + has a .json suffix - the contents have always been json. FIXES diff --git a/SCons/Tool/MSCommon/common.py b/SCons/Tool/MSCommon/common.py index fee460a9a6..16ed42cbe9 100644 --- a/SCons/Tool/MSCommon/common.py +++ b/SCons/Tool/MSCommon/common.py @@ -31,6 +31,7 @@ import re import subprocess import sys +from pathlib import Path import SCons.Util @@ -102,7 +103,8 @@ def read_script_env_cache(): envcache = {} if CONFIG_CACHE: try: - with open(CONFIG_CACHE, 'r') as f: + p = Path(CONFIG_CACHE) + with p.open('r') as f: envcache = json.load(f) except FileNotFoundError: # don't fail if no cache file, just proceed without it @@ -114,11 +116,12 @@ def write_script_env_cache(cache): """ write out cache of msvc env vars if requested """ if CONFIG_CACHE: try: - with open(CONFIG_CACHE, 'w') as f: + p = Path(CONFIG_CACHE) + with p.open('w') as f: json.dump(cache, f, indent=2) except TypeError: # data can't serialize to json, don't leave partial file - os.remove(CONFIG_CACHE) + p.unlink(missing_ok=True) except IOError: # can't write the file, just skip pass From 059f662e2991a039d2b6b3aa5997c25b1323e635 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Sat, 12 Mar 2022 10:07:59 -0700 Subject: [PATCH 3/7] PR 4111: tweak msvc cache invalid algorithm Adapt previous change so if cache entry tools dir is empty, don't invalidate the entry, it's an old compiler that doesn't add this info (we always add it, if it didn't come from the batch file it will be an empty list). We don't want to force that case to recalculate, or the value of the cache is lost for those compilers. Signed-off-by: Mats Wichmann --- SCons/Tool/MSCommon/vc.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/SCons/Tool/MSCommon/vc.py b/SCons/Tool/MSCommon/vc.py index 36a530e996..64d5d3880c 100644 --- a/SCons/Tool/MSCommon/vc.py +++ b/SCons/Tool/MSCommon/vc.py @@ -742,19 +742,24 @@ def script_env(script, args=None): if script_env_cache is None: script_env_cache = common.read_script_env_cache() - extra = f"--{args}" if args else "" - cache_key = f"{script}{extra}" + cache_key = f"{script}--{args}" if args else f"{script}" cache_data = script_env_cache.get(cache_key, None) - # brief sanity check + # Brief sanity check: if we got a value for the key, + # see if it has a VCToolsInstallDir entry that is not empty. + # If so, and that path does not exist, invalidate the entry. + # If empty, this is an old compiler, just leave it alone. if cache_data is not None: try: - cache_check = cache_data["VCToolsInstallDir"][0] - toolsdir = Path(cache_check) - if not toolsdir.exists(): - cache_data = None - except (KeyError, IndexError): - cache_data = None + toolsdir = cache_data["VCToolsInstallDir"] + except KeyError: + # we write this value, so should not happen + pass + else: + if toolsdir: + toolpath = Path(toolsdir[0]) + if not toolpath.exists(): + cache_data = None if cache_data is None: stdout = common.get_output(script, args) From ec58ef74c0be0edc138305ab95ac2f5732bb6cc1 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Sun, 20 Mar 2022 10:25:27 -0600 Subject: [PATCH 4/7] Fix use of too-new (Py 3.8) pathlib feature Signed-off-by: Mats Wichmann --- SCons/Tool/MSCommon/common.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/SCons/Tool/MSCommon/common.py b/SCons/Tool/MSCommon/common.py index 16ed42cbe9..be042f8508 100644 --- a/SCons/Tool/MSCommon/common.py +++ b/SCons/Tool/MSCommon/common.py @@ -31,6 +31,7 @@ import re import subprocess import sys +from contextlib import suppress from pathlib import Path import SCons.Util @@ -121,7 +122,8 @@ def write_script_env_cache(cache): json.dump(cache, f, indent=2) except TypeError: # data can't serialize to json, don't leave partial file - p.unlink(missing_ok=True) + with suppress(FileNotFoundError): + p.unlink() except IOError: # can't write the file, just skip pass From 7dcae31c1eaa45626718a2cef5984b200ba59af0 Mon Sep 17 00:00:00 2001 From: Joseph Brill <48932340+jcbrill@users.noreply.github.com> Date: Sun, 15 May 2022 11:56:22 -0400 Subject: [PATCH 5/7] Change cache key to tuple and read/write list of dictionaries to json --- SCons/Tool/MSCommon/common.py | 6 ++++-- SCons/Tool/MSCommon/vc.py | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/SCons/Tool/MSCommon/common.py b/SCons/Tool/MSCommon/common.py index bc7cad3d1f..591359d2e9 100644 --- a/SCons/Tool/MSCommon/common.py +++ b/SCons/Tool/MSCommon/common.py @@ -106,7 +106,8 @@ def read_script_env_cache(): try: p = Path(CONFIG_CACHE) with p.open('r') as f: - envcache = json.load(f) + envcache_list = json.load(f) + envcache = {tuple(d['key']): d['data'] for d in envcache_list} except FileNotFoundError: # don't fail if no cache file, just proceed without it pass @@ -119,7 +120,8 @@ def write_script_env_cache(cache): try: p = Path(CONFIG_CACHE) with p.open('w') as f: - json.dump(cache, f, indent=2) + envcache_list = [{'key': list(key), 'data': data} for key, data in cache.items()] + json.dump(envcache_list, f, indent=2) except TypeError: # data can't serialize to json, don't leave partial file with suppress(FileNotFoundError): diff --git a/SCons/Tool/MSCommon/vc.py b/SCons/Tool/MSCommon/vc.py index 525051cade..7f758f57c1 100644 --- a/SCons/Tool/MSCommon/vc.py +++ b/SCons/Tool/MSCommon/vc.py @@ -986,7 +986,7 @@ def script_env(script, args=None): if script_env_cache is None: script_env_cache = common.read_script_env_cache() - cache_key = f"{script}--{args}" if args else f"{script}" + cache_key = (script, args if args else None) cache_data = script_env_cache.get(cache_key, None) # Brief sanity check: if we got a value for the key, From e7e365ed20be7e7a2e0a079ca47a33416d725eca Mon Sep 17 00:00:00 2001 From: Joseph Brill <48932340+jcbrill@users.noreply.github.com> Date: Sun, 15 May 2022 14:24:42 -0400 Subject: [PATCH 6/7] Add comments describing reason for data structure conversion to/from json --- SCons/Tool/MSCommon/common.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/SCons/Tool/MSCommon/common.py b/SCons/Tool/MSCommon/common.py index 591359d2e9..e009fd5f67 100644 --- a/SCons/Tool/MSCommon/common.py +++ b/SCons/Tool/MSCommon/common.py @@ -106,6 +106,9 @@ def read_script_env_cache(): try: p = Path(CONFIG_CACHE) with p.open('r') as f: + # Convert the list of cache entry dictionaries read from + # json to the cache dictionary. Reconstruct the cache key + # tuple from the key list written to json. envcache_list = json.load(f) envcache = {tuple(d['key']): d['data'] for d in envcache_list} except FileNotFoundError: @@ -120,6 +123,9 @@ def write_script_env_cache(cache): try: p = Path(CONFIG_CACHE) with p.open('w') as f: + # Convert the cache dictionary to a list of cache entry + # dictionaries. The cache key is converted from a tuple to + # a list for compatibility with json. envcache_list = [{'key': list(key), 'data': data} for key, data in cache.items()] json.dump(envcache_list, f, indent=2) except TypeError: From 529d45c34df36355577bb0cb0abdba663c9aa596 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Sun, 15 May 2022 17:02:12 -0600 Subject: [PATCH 7/7] Drop leading '.' from cachefile name It doesn't need to be named .scons_msvc_cache.json, does not feel like it's really a "hidden" file, it'a a cache. Already renaming in this PR (adding the .json suffix) so sneak this change in as well. Signed-off-by: Mats Wichmann --- SCons/Tool/MSCommon/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SCons/Tool/MSCommon/common.py b/SCons/Tool/MSCommon/common.py index e009fd5f67..35a55b2683 100644 --- a/SCons/Tool/MSCommon/common.py +++ b/SCons/Tool/MSCommon/common.py @@ -96,7 +96,7 @@ def debug(x, *args): # SCONS_CACHE_MSVC_CONFIG is public, and is documented. CONFIG_CACHE = os.environ.get('SCONS_CACHE_MSVC_CONFIG') if CONFIG_CACHE in ('1', 'true', 'True'): - CONFIG_CACHE = os.path.join(os.path.expanduser('~'), '.scons_msvc_cache.json') + CONFIG_CACHE = os.path.join(os.path.expanduser('~'), 'scons_msvc_cache.json') def read_script_env_cache():