-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
MSVC enhancement to add all remaining msvc batch file command-line options as SCons variables #4174
Conversation
WIP PR for discussion purposes. Test SConstruct for
|
…es. Sort arguments in preferred order.
…e consistency for msvc sdk versions.
@bdbaddog and @mwichmann Guidance request at your conveniences. The "code portion" is effectively complete for this PR. However, I need some guidance on the preferred code organization moving forward. Some of the additions that I authored recently (e.g., setting up the default environment warning suppression primarily) used a large internal class with class attributes and classmethods. In the implementation of this PR, both the amount of code and the number of new internal classes expanded. While I am fond of this style, there is a drawback with the debug log as the class names are not written. I'm guessing that unit testing is a bit harder as well. As an experiment, the current branch was re-worked to use a sub-module (SCons.Tool.MSCommon.MSVC) organization with the "class namespaces" moved to proper module files. There were a few quirks regarding a clean separation of the existing vc.py code and the new submodules. For instance, the base exception type was moved to the submodule and imported by vc.py rather than defined in vc.py. The recent msvc not found policy and default environment setup were moved into their own standalone modules. The default environment setup required a callback function to avoid circular dependencies/references. This PR: https://github.com/jcbrill/scons/tree/jbrill-msvc-batchargs When comparing vc.py in both branches, once the big chunks that moved to submodules are ignored there aren't that many changes. As of this morning, both branches should be functionally equivalent. I still need to do some of my own testing for the default environment warning suppression to make sure I didn't break anything. The outstanding question is how to proceed? monolithic vc.py or smaller pieces via sub-modules? There are advantages and disadvantages of each way forward. Any and all feedback is appreciated. |
Sadly, I agree with your comment that there are advantages either way. If I'm not dealing with a transition from earlier code, I strongly prefer dividing things out into smaller pieces, and implementing separation of responsibility. We're of course dealing with transition from a long-standing code arrangement, so the "If" part clearly applies... There's nothing magical about the debug printing, if we need to add a class name or other information to how the logging setup is initialized, we can do so - but on the other hand, the msvc debug output is pretty "busy" already, at least for me it's already hard to pick out meaningful bits from lots of noise. |
Would it work to name the module I like the submodule for when a file becomes way too large.. I did similar for ninja (if you noticed it moved from ninja.py to ninja/*). Also are your complete checkboxes in the initial description at the top up to date? |
Unfortunately, the checkboxes are up to date.
README and CHANGES should not be very difficult once the other two are complete.
Probably. I'm pretty sure that importing MSCommon/vc.py as it stands now would not be different. @bdbaddog Would it be useful create a possible throwaway [WIP] PR with the modules branch so that the two of you can review the layout as it stands? If anyone likes the new layout, I can add it to this PR in one commit. If no one likes the other layout, then no harm done. |
@jcbrill - Just thinking that since MSCommon/MSVC is effectively what was previously in vc.py, it might make sense to remove vc.py and make that the module name and move it's logic into vc/init.py ? I'm ok with that refactor being in this PR. |
vc.py is still pretty large after the refactor: ~1350 loc. At present, MSCommon/MSVC, only contains "recent/new components from vc.py":
None of the MSVS/MSVC tests have been affected yet. I'm not arguing ... just feeling a little uneasy. |
Ok. Let's not tie the vc.py -> module with this functionality. |
+-> MSCommon/vc.py:_check_cl_exists_in_vc_dir: | ||
| Figure out host/target pair | ||
| if version > 14.0 get specific version by looking in | ||
| pdir + Auxiliary/Build/Microsoft/VCToolsVersion/default.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Secret Scan] It may be dangerous to commit the AWS secret access key. (view)
AWS secret access key is an important credential that allows you to use AWS programmatically.
Once this leaks, an attacker can access and operate your AWS resources.See also:
Rule Severity sider.secrets.aws.secret_access_key
warning
You can close this issue if no need to fix it. Learn more.
Sider got fooled, it thinks this might contain an AWS secret key.
The line it's flagging sure doesn't look like that to me:
| pdir + Auxiliary/Build/Microsoft/VCToolsVersion/default.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closed that one.. Weird.
Yes. Rename is fine. use |
…skip]" This reverts commit 4822e9b.
Mini-update: The only outstanding item is the request above to add content to Currently, there are no other planned code or documentation changes. While there are a lot files, when broken down into logical groups it seems more manageable as the source files are really about 1/3 of the files and some are very small. Note: due to moving the recent code to suppress warnings for msvc missing when there are no installations into Source Files [Total=15, New=11, Modified=4]:
Documentation Files [Total=5, New=0, Modified=5]:
Tests [Total=21, New=16, Modified=5]:
Generated Documentation Files [Total=4, New=0, Modified=4]:
|
@jcbrill - is this done now? |
@bdbaddog PR is complete! The wherefore and the why of the PR implementation is likely easiest to comprehend by starting the review in the following sequence:
Everything else is to support the above in one fashion or another. |
Merged. Specifically.
None of these need hold up the release, but ideally would be addressed in the near term while the code is fresh in @jcbrill's wildly productive mind. :) |
Add all remaining msvc batch file command-line options as SCons environment variables.
The intent of this PR is to enable all existing msvc batch arguments to be utilized with SCons auto-detection.
This PR modifies and/or adds the following SCons construction variables
MSVC_UWP_APP
[modifies]MSVC_SDK_VERSION
[adds]MSVC_TOOLSET_VERSION
[adds]MSVC_SPECTRE_LIBS
[adds]MSVC_SCRIPT_ARGS
[adds]With the exception of
MSVC_SCRIPT_ARGS
, all of the SCons variables have a one-to-one mapping with msvc batch file arguments.Note: the implementation of
MSVC_TOOLSET_VERSION
does not affect the selection of the msvc instances (i.e., "auto-detection") in any way. Similar to the msvc batch files: given an msvc instance, a toolset version may be specified. This distinction is important as it makes the implementation reasonably straightforward.All of the environment variables listed above are applied after the msvc instance is selected. This could be the default version of msvc if
MSVC_VERSION
is not specified.The
MSVC_SCRIPTERROR_POLICY
construction variable was added as a mechanism to report msvc batch file errors which could prove useful when attempting to diagnose mysterious build failures.Contributor Checklist:
CHANGES.txt
(and read theREADME.rst
)Source Code Tasks
MSVC_UWP_APP
CHANGES.txt
(and read theREADME.rst
)Known changes from the current main branch:
True
is accepted in addition to string'1'
MSVC_UWP_APP
in VS2013 and earlier results in an exception. Prior behavior was to silently ignore the UWP request.MSVC_SDK_VERSION
CHANGES.txt
(and read theREADME.rst
)As currently implemented, the SDK version is passed to the msvc batch files without complete validation (i.e., that the SDK version is actually installed). This is planned.
MSVC_TOOLSET_VERSION
Addresses: #3265, #3664, #4149
CHANGES.txt
(and read theREADME.rst
)Known differences between the implementation and the msvc batch files:
The implementation accepts toolset versions in the form XX.Y, XX.YY, XX.YY.Z, XX.YY.ZZ, XX.YY.ZZZ, XX.YY.ZZZZ, XX.YY.ZZZZZ.
The extended functionality consists of forcing the toolset version to be passed to the msvc batch files when not specified. Currently, there is an internal hard-coded value disabling this feature. By passing the toolset version to the batch files, the SCons cache would be impervious to toolset updates and would execute the batch files appropriately. This could be enabled via an internal-use only windows environment variable when running the test suite(s).
MSVC_SPECTRE_LIBS
CHANGES.txt
(and read theREADME.rst
)MSVC_SCRIPT_ARGS
Closes: #4106
CHANGES.txt
(and read theREADME.rst
)MSVC_SCRIPTERROR_POLICY
The msvc batch file error detection was modified to process all of the lines of stdout instead of first line. VS2017 and later, display a banner which means that no msvc batch file errors are detected if only the first line is inspected.
If msvc batch file errors are detected, the msvc script error handler is called. The scons behavior is based on the construction variable value or the global policy setting if undefined or None.
The policy values are:
The default policy is to ignore script error messages.
If script error messages are detected and
cl.exe
is not detected on the path, an internal exception is raised.Prior behavior was when an error was detected to raise an internal exception. This was typically the case when querying host/target combinations.
There are errors messages possible that are non-fatal and solely due to the difference between the "command-line environment" and the "scons shell environment" in which the batch file is invoked. These messages would be annoying and there would be no way to easily rectify the situation.
For example, with VS2017 with Typescript installed and
MSVC_SCRIPTERROR_POLICY='Warn'
, the following warning is produced: