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
Tweak the MSVC environment vars cache #4111
Conversation
- 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 <mats@linux.com>
Might be worth adding a file version to the json file? |
Probably worth noting in CHANGES and RELEASE that the first time you use the updated scons it will rebuild the cache file? |
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 <mats@linux.com>
I believe that For VS2015 and earlier, the cache would always be cleared and then populated via a call to run the script. Unless I'm way off base here (which is entirely plausible), effectively there would be no environment caching for 2015 and earlier as there would be an IndexError due to the empty list for Building with the v140 tools via VS2022 (e.g., with script args "--vcvars_ver=14.0") results in a Checking the |
Thanks for the comments. Indeed, that's why l looked for something else than digging around in PATH. The seeming natural thing would be for the "key" in the dictionary to be version-aware, but since this lookup is done based on the batch script name, it isn't. Would it make sense to leave the retrieved value alone if the VCToolsInstallDir came back as an empty list? |
I'm not sure there is a bulletproof (or even reliable) solution. Just thinking out loud: a synthetic key could be added to the cache entry prior to writing that contains a (possibly combined) list of path elements (e.g., |
... or the synthetic key and values could just be part of the environment. |
There is another "fly in the ointment" so to speak: current SCons uses the default/latest toolset which become part of the path. For example:
If a MSVS update were performed that installed a newer toolset while keeping the old toolset, the stale cache entry would still be valid but would not be what a user expects (i.e., the latest installed toolset). If the toolset version were always passed to the msvc batch files, this particular problem effectively disappears. |
Yes, I was worrying about that too. It's perhaps a little artificial, but I actually have a test setup that keeps the last three toolsets so yes, triggering on "directory missing" is not strictly accurate. This issue wasn't introduced by this patch, since that would have been true before - we would have retrieved the entry matching the key, it would be found, would silently work - and would not be the latest version if the previous one were kept. |
SCons could always pass the toolset version for the default version:
In this way it becomes part of the cache key and is consistent. Although now there is another problem... |
Ouch, something in this change caused an overly long windows path to be constructed in the environment the CI system sees (worked for me). Don't bother reviewing change 65bdbc9 - I'll fix and update when I get a chance. |
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 <mats@linux.com>
65bdbc9
to
059f662
Compare
Okay, I accidentally committed an experimental change in a different file that hadn't intended to. Repushed, let's see if that's better. |
What follows is a suggestion, some observations, and a detailed example of cache file manipulation and when it could be useful. The current separator between the script name and the arguments is a sequence of two hyphen characters (
The hyphen is legal in both filenames and as batch file arguments. Currently, a user-defined batch file is checked for existence which automatically rules out any characters that are not allowed in windows file names. Modern msvc batch files accept most parameters with a prefix of Perhaps using a character that is not legal in a filename would make it easier outside of SCons to process the json cache file and split the cache keys into their component batch file name and arguments. For example, using the pipe character (
This should guarantee that the cache key could be partitioned (e.g., Some notes to file away for future reference:
A useful technique for a build server would be to "prime the msvc cache" via a batch file and an SConstruct file that calls SCons to regenerate the cache file with all msvc batch file invocations that will be used for production builds. This could be run immediately after msvc updates. The following example consists of deleting the existing cache, generating a new cache, re-writing the cache due to an msvc bug for MSVC 14.1 arm64 targets, and then testing the cache by re-running the same sconstruct that generated the cache. The MSVC 14.1 arm64 targets fail with the most recent update to SDK 10.0.22000.0 due to an instrinsics issue. The workaround is to use SDK version 10.0.20348.0 or earlier. The example demonstrates how to pre-populate and then process the scons cache file to re-write the 14.1 arm64 targets to use SDK 10.0.20348.0 for default builds. I am not saying this is a good idea, just an outside-the-box example of possible utility. SConstruct:
Re-write cache script:
Batch file (using a work-in-progress copy of Scons):
Stderr output log:
In the second pass, the cache is used for all builds including the "default" 14.1 and 14.1Exp arm64 target builds with SDK Pre-populating the cache could be useful on build servers after msvc updates. |
One thought as a comment to @jcbrill 's recent comment could we skip constructing a hash key by building a string, and instead just use a tuple? (They're hash'able as they're immutable (correct me if I'm wrong)) |
@bdbaddog Yes it can. However, a tuple is serialized to a list with json. The list would have to be converted to a tuple when loading the cache. This could be done in a dictionary comprehension. |
It's not a secret that this whole thing was a hack - I run the SCons testsuite a lot, and running 1000+ distinct tests where many of them have to go through the msvc setup dance was a real pain. Stuffing this thing in on an experimental basis helped me in running those tests on Windows, though I don't spend that much time there. I'm more than happy to consider changes to help more maintstream usage. |
A couple thoughts:
Sounds like from @jcbrill 's comment above that it's fine for 2015 and newer, but for MSVC's older it's going to invalidate the cache unnecessarily. |
I feel your pain. Some of my test sconstruct files run though 240+ msvc batch files. While I'm sure it is not as bad as your case, a single invocation takes about 5-6 minutes.
a) Yes
If I'm reading the code correctly, this is already achieved. At the risk of annoying @mwichmann more than I already have:
If my count is correct, it would take approximately two new lines of code and changes to three lines of code. While testing the proposed changes below, it was revealed that the For my own work outside of scons, I am keen to see these changes work their way into scons. vc.py fragments:
I have to think about it some more, but it should be possible to write a batch file/python script/sconstruct file that reads the existing cache, extracts the scripts, deletes the cache file, and re-runs all of the existing batch files to regenerate the cache. The idea being that every time MSVC is updated, manually run the process to automatically update the cache. Common.py fragments:
This is reasonable. With the exception of the standalone script I'm working on, most of the code snippets posted are throw-away efforts to demonstrate functionality. If you think there is value, I'm willing to work with you on this. If we can defer this in the short term, I would be grateful. I have one more PR that is nearly ready that I would like to get issued and hopefully accepted which makes most of the my work outside of scons usable in scons via a standalone script placed in, or symlinked from, site_scons. The standalone script could be useful to others and will eventually make its way into its own repository. The innards go through bouts of wholesale changes but the API usable from scons is fairly stable with only minor changes planned. |
Not annoying!
These look reasonable to me, although I have a silly preference for using Pathlib over the os.path stuff. I'll probably add these changes in soon. Anyway:
Oops, okay I'll update this bit right away. |
Signed-off-by: Mats Wichmann <mats@linux.com>
Note for future reference: for MSVS 2015 and later, the msvc system environment includes path elements based on the latest SDK version number (e.g., 10.0.22000) at the time of invocation. All other things being equal, simply installing a new Windows SDK invalidates the cache entries for 2015 and later. |
Are the older version still there and usable but not the latest (and thus invalidating the cache if it's meant to use latest)? |
I believe the "correct" answer is that it is dependent on the user's installation. It may not be the same across all users. Both situations are possible depending on the user's installation habits. There are check boxes for each SDK version in the Visual Studio installer. I'm not sure if de-selecting an SDK version in the installer removes it from the hard drive or not. The windows SDKs (by version) also appear in both the windows control panel Programs and Features list and the windows settings apps list so it appears to be possible to uninstall individual SDKs from the control panel and/or settings. The way the 2022 msvc batch file is written, it appears that unless the user specifies a specific SDK version to use as a command-line argument, the latest SDK version is selected provided that it meets the minimal requirements (app vs desktop, etc). The basic process in a nutshell: the SDK folder is retrieved from the registry, the include folder directory names are sorted and walked via a dir command, folders beginning with
At best, the cache would contain "stale" entries but the environments would contain references to an older SDK that is still present. At worst, the older SDK was uninstalled and the "stale" entries would refer to non-existent paths. It should be possible to verify the cache entries for the SDKs but it is likely not as straightforward as one might hope. |
it appears to (I've done this before), though there's a cache of msvc "bits" unless you use some special options to avoid the cache. My vm's which don't have what you'd consider a "real" amount of disk, fill up even when I remove stuff. Sigh.
I have done this also - for the same dumb reason, out of space, it's easier to nuke an sdk from Programs quickly than from the vs installer - and it's easy to bring back using the installer. |
For your amusement and/or horror: a standalone script was developed to test the cache file name options and is available in the linked repository below. I'm not necessarily saying it is a good idea/method but may be a little more robust in the face of user input. If nothing else, it may be easier to test some ideas outside of SCons. Unsolicited cache file name handling features:
Code fragment (likely containing useless comments): |
Horror, maybe: I didn't expect this "quick hack to help my own sanity waiting for the testsuite to run on Windows" thing to take on this kind of life - wondering if it's even worth all this effort? At a quick glance, this looks excellent... I do like that a "false-like" value actually means something if "true-like" does. and yes, integrating this with the rest of this subsystem in using logging in debug mode to record useful information is good, too. |
Probably not. "How hard could it be?" is usually where it goes off the rails... |
I run the MSVC+MSVS tests frequently. With so many versions of msvc installed, the cache for just running the msvc/msvs tests has 21 entries. It is a time saver and useful to me. I appreciate that you added the cache. |
Is the tuple-ification of the key planned? |
Sure. Will work on it tomorrow. Need to find and massage source reference implementation as it was not in either SCons or this branch. |
Code available via https://github.com/jcbrill/scons/tree/mwichmann-msvc-cachefix Issued a PR to https://github.com/mwichmann/scons/tree/msvc/cachefix Feedback is welcome. |
Change cache key to tuple and read/write list of dictionaries to json
Merged - thanks. Let's see how the build does, then figure out next steps. |
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 <mats@linux.com>
Post-merge update: confirmed that old and new cache files are incompatible - if you force an old cache file to be picked up by the current code, it will take a |
can you add another except clause and message? |
Thinking out loud... tt should be possible to detect if a list or dict was returned from json. If a dict, old format and convert to list and new format. Maybe? I would be happy to try later today. |
"detect and baul out with msg" would be okay with me, fwiw. not sure we want to carry conversion code around. |
Something like this:
Output:
|
Yes. that's what I was thinking. |
Perhaps not optimal, but once detected that the cache is invalid a warning could be issued indicating the cache was invalid and the CACHE_CONFIG variable could be reset to None which would effectively disable the cache and prevent overwriting but the build would continue. Unless the user removed the cache, the same thing would happen the next time. Another alternative is to issue a warning that the cache is invalid on the read, but continue on and overwrite the cache during the run. In this case it is self-healing which is all a user is going to do anyway. I'm a little wary of overwriting the cache file. And so it goes.. |
I'd leave the filename and just overwrite it with whatever is detected per usual... |
How about a warning and continuing? Cache file likely overwritten. At present, this really only handles the old format change from dict to list. With a warning, at least a user might understand why the run is taking longer. Warning sent to debug output if enabled as well.
Partial output:
|
Looks good to me, can you PR it? |
#4152 so it's linked here |
Since this feature is still marked experimental, and these aren't exactly earthshaking changes (basically, will recompute the cache data once), they should be okay without any kind of a transitional cycle.
Questions for reviewers:
cache_data["VCToolsInstallDir"]
, which according to the comments isn't used by much, but it seemed to be the only place that had a distinct path value one could check to see if it still exists. Open to alternative suggestions.Signed-off-by: Mats Wichmann mats@linux.com
Contributor Checklist:
CHANGES.txt
(and read theREADME.rst
)