MSCommon: minor changes to prevent possible inconsistencies with the msvc runtime and/or external file cache. #4757
Conversation
…event using runtime modified values Changes: - Rename VS_VC_VARS to _MSVC_ENVIRON_IMPORT. - Rename KEEPLIST to _MSVC_ENVIRON_PRESERVE. - Change imported variables from a list to a tuple. - Change file level variable reference in get_output function to optional import_vars argument default value. - rename optional keep argument in parse_output function to keep_vars. - Update comments.
|
@jcbrill - so if I understand this PR the idea is to block users ability to reach in and tweak internals on the chance that doing so may yield incorrect msvc cache? |
Correct (more or less). The setup configuration returned to the user may not take into account the user's changes based on the runtime and/or filesystem cache.
No. The potential issue is caused by the request (starting at #4717 (comment)) to move the list of variables that was defined inside a function to file level. Prior to the move, it was impossible for a user to modify the list of variables. The variable list move was not the result of needed functionality or user request. This is one of those issues that makes the msvc code difficult to comprehend. What seems to be innocuous has very subtle interactions with the existing code. There were similar issues (which have been since been fixed) with the original implementation of the Until a reasonable solution for cache-consistency (runtime and filesystem) is found, it better just to disallow it for now. |
|
I disagree. If you're monkey patching an internal variable, buyer beware. Add a big comment near the list in the source code that if you change this list you should remove your existing msvc cache file, and call it a day. It's not like we're documenting this usage, or providing a documented API to change this, this is reaching into the internals. If we were, then 100% we should insure that usage by users couldn't cause the inconsistencies you're concerned about. |
|
Then I guess there is no need for this PR. Closing soon. |
|
Closing. |
3 participants
The list of variables imported from the OS environment and added to the msvc environment was moved from inside a function to file-level in #4717 which could enable a user to extend the list of imported variables. This could yield unexpected behavior with the msvc runtime and/or external file cache.
The msvc cache key consists of the msvc batch file and the msvc batch file arguments. The cache contents are based on both the list of variables imported from the OS environment and the list of variables preserved from the msvc environment. Additions to the list of variables imported from the OS environment could introduce runtime and/or external file cache consistency issues. Under certain circumstances, due to runtime and/or external file caching, additions to the list of variables imported from the OS environment could be ignored entirely.
The tuple of variables retrieved from the msvc environment and added to the SCons environment is defined at file-level. The tuple of variables is specified as the default value of an argument in the function to process the msvc output. The optional argument is not passed from the vc code. As written, it is effectively impossible to extend the tuple of preserved variables and have the extended variables used within the function to process the msvc output.
The variables imported from the OS environment was changed from a list to a tuple.
The tuple of variables imported from the OS environment and added to the msvc environment is defined at file-level. The tuple of variables is now specified as the default value of an argument in the function to get the msvc output. The optional argument is not passed from the vc code. As written, it is effectively impossible to extend the tuple of imported variables and have the extended variables used within the function to get the msvc output.
This PR effectively restores the behavior of the imported and preserved variables list behavior to the behavior prior to acceptance of #4717 until the cache consistency issues can be addressed.
There may be some relevant discussion in #4727 which was abandoned.
This PR is internal only with nothing user-facing.
Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).