Fix 4609: MSVS tests and minor changes to msvs tool

[jcbrill]

Fix MSVS tests and minor changes to Tool/msvs.py.

testing/framework/TestSConsMSVS.py:

• Add default project GUID
• Pass MSVS_PROJECT_GUID via environment
• Add AdditionalOptions Condition to expected vcx project file
• Fix vs version number for vc version 14.3
• Fix expected platform toolset version

SCons/Tool/msvs.py:

• Use environment MSVS_PROJECT_GUID when creating project files info
• Fix writing Visual Studio 15 for VS2015
• Add .vcxprof as an expected suffix for assigning the name to the file base name

Fix early exit after vc version 8.0 (exit at the end of first loop execution) in:

• test/MSVS/vs-files.py
• test/MSVS/vs-scc-files.py
• test/MSVS/vs-scc-legacy-files.py
• test/MSVS/vs-variant_dir.py

Tests:

• Modify tests using TestSConsMSVS to add MSVS_PROJECT_GUID to the environment in the generated SConstruct/SConscript files.
• Fix: delete env['PYTHON_ROOT'] before next loop iteration in test/MSVS/vs-files.py.
• Fix: add safe HOST_ARCH to sconstruct/sconscript files for vs*scc-files` and vs*-scc-legacy-files tests for non-windows, unsupported architectures.

Fixes #4609
Fixes #4608
Fixes #4612

Contributor Checklist:

• I have created a new test or updated the unit tests to cover the new/changed functionality.
• I have updated CHANGES.txt and RELEASE.txt (and read the README.rst).
• I have updated the appropriate documentation

[jcbrill]

@mwichmann Need review of Tool/msvs.py changes to function GenerateProjectFilesInfo.

I believe all elements of the loop should share the same project GUID but I'm not sure. Passing a project GUID via the environment was already supported. However, I'm not convinced that it was ever used to test the newer generated files due to the early exit issue.

There may be a better/safer way by possibly mapping the project guid to the first generated loop GUID and using the project GUID only for matching generated loop GUIDs. The tests would certainly fail if there were different GUIDS in the loop.

[jcbrill]

There is an issue with the Tool/msvs.py changes to function GenerateProjectFilesInfo.

It works as expected for the test suite and would be fine in production if there is only one project.

It's not going to do the right thing in production for multiple projects (i.e., len(dspfiles) > 1) as they would all get assigned the same project GUID.

In this case, it would be more useful if MSVS_PROJECT_GUID was a scalar GUID or list of GUIDs.

[jcbrill]

@mwichmann The is an odd test/Docbook/basic/html/html_cmd.py failure.

350/1285 (27.24%) C:\Python38\python.exe test\Docbook\basic\html\html_cmd.py
STDOUT =========================================================================
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
C:\ProgramData\chocolatey\bin\xsltproc.EXE -o manual.html C:\projects\scons\SCons\Tool\docbook\docbook-xsl-1.76.1\html\docbook.xsl manual.xml
scons: done building targets.
STDERR =========================================================================
0a1,7
> error : No such file or directory
> http://www.oasis-open.org/docbook/xml/4.1.2/dbcentx.mod:192: warning: failed to load external entity "http://www.oasis-open.org/docbook/xml/4.1.2/ent/iso-pub.ent"
> %ISOpub;
>         ^
> Entity: line 1: 
>  %ISOpub; 
>          ^
FAILED test of C:\projects\scons\scripts\scons.py
	at line 734 of C:\projects\scons\testing\framework\TestCommon.py (_complete)
	from line 835 of C:\projects\scons\testing\framework\TestCommon.py (run)
	from line 459 of C:\projects\scons\testing\framework\TestSCons.py (run)
	from line 41 of test\Docbook\basic\html\html_cmd.py
Test execution time: 25.5 seconds

#4609 failed the same test:

350/1285 (27.24%) C:\Python38\python.exe test\Docbook\basic\html\html_cmd.py
STDOUT =========================================================================
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
C:\ProgramData\chocolatey\bin\xsltproc.EXE -o manual.html C:\projects\scons\SCons\Tool\docbook\docbook-xsl-1.76.1\html\docbook.xsl manual.xml
scons: done building targets.
STDERR =========================================================================
0a1,7
> error : No such file or directory
> http://www.oasis-open.org/docbook/xml/4.1.2/dbcentx.mod:192: warning: failed to load external entity "http://www.oasis-open.org/docbook/xml/4.1.2/ent/iso-pub.ent"
> %ISOpub;
>         ^
> Entity: line 1: 
>  %ISOpub; 
>          ^
FAILED test of C:\projects\scons\scripts\scons.py
	at line 734 of C:\projects\scons\testing\framework\TestCommon.py (_complete)
	from line 835 of C:\projects\scons\testing\framework\TestCommon.py (run)
	from line 459 of C:\projects\scons\testing\framework\TestSCons.py (run)
	from line 41 of test\Docbook\basic\html\html_cmd.py
Test execution time: 25.5 seconds

[mwichmann]

There's a range of things to do with docbook, that seem to get gradually worse over time. That one seems to indicate a missing file, but I've seen the same test fail other ways on systems that do have the file. It seems Debian/Ubuntu don't package it either (according to packages.ubuntu.com) - have no idea where it would come from if you're on Windows. Wouldn't worry too much. Someday, maybe someone will fix those.

[jcbrill]

Thank you.

There is a lengthy commit message for the last commit: 76839d9

I believe there were two unreported bug fixes and a minor change in behavior in the commit.

One of which appears to have happened in the real world:
https://stackoverflow.com/questions/41637580/how-to-correclty-use-msvssolution-and-msvsproject

[bdbaddog]

Replace the entire header here with what's in template/test.py

[jcbrill]

should be fixed

[bdbaddog]

Replace the entire header here with what's in template/test.py

[bdbaddog]

Replace the entire header here with what's in template/test.py

[bdbaddog]

Replace the entire header here with what's in template/test.py

[jcbrill]

Should be fixed

[bdbaddog]

Replace the entire header here with what's in template/test.py

[jcbrill]

Should be fixed.

[bdbaddog]

Replace the entire header here with what's in template/test.py

[bdbaddog]

Please add a RELEASE/CHANGES.txt blurb.

@mwichmann & @jcbrill - do we need any doc updates here? Any functional changes not covered by docs?

[jcbrill]

do we need any doc updates here? Any functional changes not covered by docs?

YES!

An optional argument was added to MSVSProject (e.g., projectguid="{MYUSERGUID}"). It was either add an argument to MSVSProject or change the behavior of MSVS_PROJECT_GUID (which is documented) to be either a scalar or a list.

With the addition of the project argument, MSVS_PROJECT_GUID is somewhat obsolete and should only be used for single project solutions.

However, it is still used and may not do the what one would want if MSVS_PROJECT_GUID is defined and there are two or more MSVSProject instances without projectguid settings attached to MSVSSolution as multiple projects would be using the same GUID.

There is also a known change in behavior for standalone MSVSSolution generation: the solution file has the project files as sources due to the GUID assignments (i.e., the projects are generated before the solution. This was not the case before so it affects the behavior of clean: when the solution is cleaned the project files are also cleaned (this is the same behavior as auto_build_solution=1).

There is one more thing I want to look into involving cleaning with the variant-dir builds. The behavior is that the placeholder files are removed but not the files generated in the source folder. I think there is an old issue for this.

It would be nice if someone actually using MSVSProject and MSVSSolution tested the behavior.

[bdbaddog]

You do realize if that:
env.MSVSProject(..., MSVS_PROJECT_GUID='unique to this builder call')

Will yield that builder getting it's own unique MSVS_PROJECT_GUID (via the OverrideEnvironment() which is always created when a builder is called with extra arguments)

So no need to add projectguid.

-Bill

[jcbrill]

The "projectguid" is retrieved from the environment. It needs to be added to the MSVSProject arguments documentation.

[jcbrill]

Need to review the documentation AND the code.

[jcbrill]

The project guid was implemented parallel to the MSVSProject argument slnguid which does not appear to documented anywhere but is retrieved in the msvs tool.

slnguid is still in all of the msvs test files.

For example, runfile.py

SConscript_contents = """\
env=Environment(tools=['msvs'], MSVS_VERSION = '8.0')

env.MSVSProject(target = 'Test.vcproj',
                projectguid = '%(PROJECT_GUID)s',  <=== ADDED IN THIS PR
                slnguid = '{SLNGUID}',
                srcs = ['test.cpp'],
                buildtarget = 'Test.exe',
                runfile = 'runfile.exe',
                variant = 'Release',
                auto_build_solution = 0)
"""

Ultimately, the decision is above my paygrade.

The documentation for MSVS_PROJECT_GUID is pretty simple and there is no explicit reason to believe one would add it to MSVSProject instead of the Environment.

For single project solutions it would not matter:

env = env=Environment(tools=['msvs'], MSVS_VERSION = '8.0', MSVS_PROJECT_GUID="{MYGUID}")

env.MSVSProject(target = 'Test.vcproj',
                srcs = ['test.cpp'],
                buildtarget = 'Test.exe',
                runfile = 'runfile.exe',
                variant = 'Release',
                auto_build_solution = 1)

For multiple projects, this would assign the same GUID to both projects:

env = env=Environment(tools=['msvs'], MSVS_VERSION = '8.0', MSVS_PROJECT_GUID="{MYGUID}")

p1 = env.MSVSProject(target = 'Test.vcproj',
                     srcs = ['test.cpp'],
                     buildtarget = 'Test.exe',
                     runfile = 'runfile.exe',
                     variant = 'Release',
                     auto_build_solution = 0)

p2 = env.MSVSProject(target = 'Test.vcproj',
                     srcs = ['test.cpp'],
                     buildtarget = 'Test.exe',
                     runfile = 'runfile.exe',
                     variant = 'Release',
                     auto_build_solution = 0)

env.MSVSSolution(
    target = 'Test.sln',
    projects = [p1, p2],
    variant = 'Release',
)

Where this would allow projectguid to be documented with MSVSProject:

env = env=Environment(tools=['msvs'], MSVS_VERSION = '8.0')

p1 = env.MSVSProject(target = 'Test.vcproj',
                     projectguid = '{MYGUID1}',
                     srcs = ['test.cpp'],
                     buildtarget = 'Test.exe',
                     runfile = 'runfile.exe',
                     variant = 'Release',
                     auto_build_solution = 0)

p2 = env.MSVSProject(target = 'Test.vcproj',
                     projectguid = '{MYGUID2}',
                     srcs = ['test.cpp'],
                     buildtarget = 'Test.exe',
                     runfile = 'runfile.exe',
                     variant = 'Release',
                     auto_build_solution = 0)

env.MSVSSolution(
    target = 'Test.sln',
    projects = [p1, p2],
    variant = 'Release',
)

Even for single projects, it would be more useful:

env = env=Environment(tools=['msvs'], MSVS_VERSION = '8.0')

env.MSVSProject(target = 'Test.vcproj',
                projectguid = '{MYGUID}',
                srcs = ['test.cpp'],
                buildtarget = 'Test.exe',
                runfile = 'runfile.exe',
                variant = 'Release',
                auto_build_solution = 1)

I like it better as lowercase argument like the other project arguments. Having both is probably asking for trouble.

[bdbaddog]

There's no projectguid in the manpage, only MSVS_PROJECT_GUID, so that should be the variable name used.

I think you're still missing my point about OverrideEnvironment()

env = env=Environment(tools=['msvs'], MSVS_VERSION = '8.0', MSVS_PROJECT_GUID="{MYGUID}")

p1 = env.MSVSProject(target = 'Test.vcproj',
                     srcs = ['test.cpp'],
                     buildtarget = 'Test.exe',
                     runfile = 'runfile.exe',
                     variant = 'Release',
                     auto_build_solution = 0)
# Or you can specify MSVS_PROJECT_GUID above and skip the declaration when you specify Environment() above.

p2 = env.MSVSProject(target = 'Test.vcproj',
                     srcs = ['test.cpp'],
                     buildtarget = 'Test.exe',
                     runfile = 'runfile.exe',
                     variant = 'Release',
                     auto_build_solution = 0,
                     MSVS_PROJECT_GUID="{MYGUID_for_proj_2}")

env.MSVSSolution(
    target = 'Test.sln',
    projects = [p1, p2],
    variant = 'Release',
)

[jcbrill]

I think you're still missing my point about OverrideEnvironment()

I'm not missing the point.

There's nothing on the man page about projectguid because I added it in this PR and was planning on adding it to the argument list for MSVSProject where the documentation of the variable would be with its primary usage.

The documentation of MSVS_PROJECT_GUID is unclear and a search of all the files will reveal that the only python source file that contains it is in msvs.py. It does not appear in any of the tests. On the other hand slnguid appears in all of the tests files but is not documented.

I will remove projectguid and adjust all the tests accordingly.

[bdbaddog]

But you were asserting For multiple projects, this would assign the same GUID to both projects: indeed that's true with the code, but you can specify any envvar as extra argument to any builder and that invocation of the builder would see that value and not that in the env it was called with.

[jcbrill]

But you were asserting For multiple projects, this would assign the same GUID to both projects: indeed that's true with the code, but you can specify any envvar as extra argument to any builder and that invocation of the builder would see that value and not that in the env it was called with.

Everything you said is correct.

How would a new user reading the man page know that MSVS_PROJECT_GUID should be used with MSVSProject and not the Environment?

This could be problematic if one started off with one project and then later added more projects.

Anyway, I've removed projectguid and updates all of the MSVS test files. Running WSL and Windows MSVS tests before committing and pushing.

[bdbaddog]

@jcbrill - it's in the docs that you can do this. you can specify CPPPATH, LIBS, LIBPATH,etc to Program(), etc..

https://scons.org/doc/production/HTML/scons-user.html#builder_overrides

[jcbrill]

Don't ping me for the MSVS issues :)

[bdbaddog]

Sorry if you're taking offense here, it seemed like you weren't aware that this was standard practice (to override or specify env vars to builders in the builder call)..

[jcbrill]

Sorry if you're taking offense here, it seemed like you weren't aware that this was standard practice (to override or specify env vars to builders in the builder call)..

Apology not necessary. No offense taken.

<petulant rant>
There are a number of features in the msvs tool around MSVSProject and MSVSSolution for which there were no tests nor were all combinations of features addressed and likely still aren't. Multiple projects with a single solution is one instance. Combinations of feature tests that were missing resulted in #4612, and #4613. Nor were there documentation examples.

When I added a multiple project single solution test using a variantdir from scratch, I couldn't understand why it wasn't working. A google search a user in the wild experiencing the same behavior: (https://stackoverflow.com/questions/41637580/how-to-correclty-use-msvssolution-and-msvsproject).

It is a bit annoying that particular use-case was not tested. I would have expected that to be somewhat popular. Multiple projects with a single solution does work as expected when not using a variantdir though.
</petulant rant>

[bdbaddog]

@jcbrill - I don't think MSVSSolution/Project are a widely used feature, especially with VariantDirs. And even less SCons developers use those.. so I'm to terribly suprised that the solution isn't as flexible as large real world usage would demand (as yet)..

[mwichmann]

Fixes #4608 [needs testing]

I can now confirm that it does. Got quite a bit slower, now those four tests are running all the combos, not just one :-)

[mwichmann]

Fixes #4608 [needs testing]

I can now confirm that it does. Got quite a bit slower, now those four tests are running all the combos, not just one :-)

Still think these tests could be skipped on non-Windows platforms... as noted elsewhere, these days we have adequate automated test coverage on Windows, and with the retirement of Visual Studio for Mac, we could limit these to the only platform they've of intetrest on.

I did run across a project recently that uses the generated vsxproj file... might have been Godot, or it might have been that automotive project, but can't remember at the moment. It does seem to be the case that people use it to inform the IDE about the project details but don't depend on using it to be able to build directly from Visual Studio using SCons.

Just checked Godot... they seem to create their own files as well (there's a directory of project file templates)

[jcbrill]

It is probably valuable for some as a code browser if nothing else.

Mongodb appears to generate their own project file (vcxproj).

From earlier testing, I'm reasonably confident platform="win32" can be removed from the Environment() calls in the MSVS tests and it still generates code as expected.

First pass: remove platform="win32"
Second pass: skip if not windows

Either way, the first pass can be done. I'm onboard with the second pass as well.

I have an almost-working example of generating the sln/vcxprof files in their own directory in user-space (sconstruct/sconscript files). It is not as straightforward as one might hope due to needing to having to generate relative file paths for the source files. I suspect that is why the files are generated in the source directory.

Known issue: the generated project files include the top-level "SConstruct" file as a source file without a relative path to the sconstruct file.

There are quite a view outstanding Issues for MSVSProject/MSVSSolution. At least one of them (not sure the issue number at the moment), I believe is "invalid usage" (i.e., SCons is correct to complain) rather than a bug.