-
-
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 script enhancements including user-defined arguments #4106
Comments
For item 3, it should not overload So let's split that entirely from the discussion of providing the ability to pass arguments to the specified script.(Item 1) For item 2- As always thanks for your thorough and complete discussion above. |
Agreed.
Fair enough. Prior to a PR for script arguments, the code relevant to @bdbaddog Would it be OK to open another feature request issue for discussion purposes?
Works for me. I have updated the variable name in my standalone script imported from site_cons and called from SConstruct to populate the environment to demonstrate capability.
That is very kind. I am struggling with one line in the existing documentation for Definitions of the existing and proposed environment variables:
The detection algorithm priority sequence is straightforward:
Revised and tested proposed implementation for Items 1 and 3:
|
I'm not sure. MSVC_USE_SCRIPT_ARGS probably should accept a scalar string or a list of strings. A list of string arguments can be more palatable from a user perspective. Also, it would likely be a good idea to verify the argument is in fact a string or that the flattened list contains all strings and otherwise raise a user argument exception immediately with a sensible error message. It is probably bad news to pass an incorrect type down through the script environment calls. This is what I have been using in my implementation:
|
Off-topic trivia:
Any guesses? |
I already know the answer. This nicely encapsulates the issues with debugging this sort of problem. The crux of the problem is the aggressively minimal SCons environment in which the msvc batch files are executed: when run directly from the command-line there are no errors/warnings. To add insult to injury, even when the MSCommon debugging is enabled, the "raw" stdout is not logged. SCons does not include some of the windows system environment variables used by the setenv.cmd batch file. SetEnv.cmd fragment:
The PROCESSOR_ARCHITECTURE and PROCESSOR_ARCHITEW6432 environment variables are not defined. This causes the else clause to be executed. On 64-bit platforms, the x64 compiler is installed in the x64 folder. On 32-bit platforms, the 64-bit compiler is installed in the x86_x64 folder. Therefore, x86_x64 does not exist and the message is echoed to stdout. The message "The x64 compilers are not currently installed" is in fact generated when executed from SCons. If the output were captured it would look something like this (taken from my own tools):
The warning is innocuous and generated because the "OS" environment variable is not defined. The older files expect OS to be defined. This is why the msvc 6.0 paths are in quotes: the batch file thinks it is 95/98/ME since OS is not "Windows_NT" In addition, the SDK setenv.cmd batch files expect delayed expansion to be enabled which is typically not the default in a windows shell. The /x86 target environment is "incomplete" but works anyway. Here is the resulting environment for the x86 invocation:
Due to delayed expansion not being enabled, !PATH!, !Include!, and !Lib! are not expanded. Also, the Framework path is incomplete due to "windir" not being defined. SDK 6.0 contains stand-alone msvc 8.0 compilers. I have a mind-bending intermediate windows batch file that sets the environment as appropriate in a setlocal/local block, calls the SDK batch file, and "exports" the non-windows wow64/system variables back into what would be the msvc batch file environment. If interested, I could attach. |
Status update:
|
With the recent PR proposing adding arguments to user-defined script/batch files (MSVC_USE_SCRIPT can pass in args #4102), perhaps it is worthwhile to revive the discussion had earlier in PR #4063.
There are three potential enhancements to msvc usage that would enhance the user experience:
In the interest of full disclosure: the author also has a code branch ready implementing Items 1 and 3 above but has not updated the requisite documentation yet. Implementation of Item 2 in the current SCons code base is not as straightforward as Items 1 and 3 and has been deferred to a later date.
The first two items make available to an SCons end-user all of the msvc batch file options available to command-line users. The first two items effectively provide a "universal escape hatch" which also reduces the burden on SCons development to "catch-up" with currently unsupported features and/or any new shiny features that may be added to msvc batch files in the future.
The third item is a middle ground between the all (SCons msvc detection via restricted system execution environment) or nothing (user-populated environment) currently offered. While the utility may not be readily apparent, the reader is encouraged to keep an open mind before rushing to a snap judgment. Additional support material will be provided in future posts.
The user-provided content for Item 1 (script arguments) and Item 2 (pass-through script arguments) are not likely to be the same as "native" features are added to SCons. In the current SCons implementation, the store/UWP argument is available via the MSVC_UWP_APP variable while a user-defined script would need to use the "store" argument. If toolset support were added to SCons, the script argument(s) would still need the toolset specification while the pass-through argument(s) would not.
It would likely be best if there are two distinct environment variables for user-defined script arguments and user-defined pass-through arguments:
Item 1 - MSVC_USE_SCRIPT_ARGS rationale:
Item 2 - MSVC_ADD_ARGS or MSVC_PASS_ARGS notes:
Item 3 - ENV-like dictionary notes:
The relevant posts from closed PR #4063 are quoted here for convenience (note that the proposed implementation details have changed since originally posted):
For discussion purposes, the author's proposed implementation for Items 1 and 3 is:
Comments:
The text was updated successfully, but these errors were encountered: