msvc tests fail on 32-bit ARM host
Closed this issue · 30 comments
Ran the testsuite on an armv7l host (Linux OS - specifically, an old Raspberry Pi), and the tests of generating the Visual Studio Project files fail.
This is a little awkward... it's supported to generate code for an armv7 target on Windows, but armv7 hosts are not, so it's not in our lookup tables. But we're not actually using MSVC for anything in this test (as far as I know), still we end up passing through the logic. I asked on Discord whether there was any point to these non-live tests running on non-Windows platforms, and it's considered to still have some value - they exist for historical reasons when availability of msvc for test systems were poor, as the community versions either didn't exist, or were limited ("express"), so it was felt valuable to "test what you can" on any host. But now we're running through the tool setup code and falling afoul of an unsupported combination when we don't even care about that combination for compiler purposes. Not sure where to take this, so just recording it. I presume this means the tests will now fail on other obscure architectures like SPARC, MIPS, PowerPC. RISC-V (the last one we might actually encounter for real someday if its popularity keeps growing).
Failures look like this:
MSVCUnsupportedHostArch: Unrecognized host architecture 'armv7l':
File "/tmp/scons/testcmd.23855.tmz54cn3/SConstruct", line 1:
env=Environment(platform='win32', tools=['msvs'], MSVS_VERSION='8.0',
File "/home/mats/github/scons/SCons/Environment.py", line 1282:
apply_tools(self, tools, toolpath)
File "/home/mats/github/scons/SCons/Environment.py", line 120:
_ = env.Tool(tool)
File "/home/mats/github/scons/SCons/Environment.py", line 2095:
tool(self)
File "/home/mats/github/scons/SCons/Tool/__init__.py", line 265:
self.generate(env, *args, **kw)
File "/home/mats/github/scons/SCons/Tool/msvs.py", line 2087:
msvc_setup_env_once(env, tool=tool_name)
File "/home/mats/github/scons/SCons/Tool/MSCommon/vc.py", line 2211:
msvc_setup_env(env)
File "/home/mats/github/scons/SCons/Tool/MSCommon/vc.py", line 2410:
d = msvc_find_valid_batch_script(env,version)
File "/home/mats/github/scons/SCons/Tool/MSCommon/vc.py", line 2244:
platforms = get_host_target(env, version)
File "/home/mats/github/scons/SCons/Tool/MSCommon/vc.py", line 642:
host_platform = get_host_platform(host_arch)
File "/home/mats/github/scons/SCons/Tool/MSCommon/vc.py", line 580:
raise MSVCUnsupportedHostArch(msg % repr(host_platform)) from None
This particular snip came from
from line 103 of /home/mats/github/scons/test/MSVS/vs-scc-files.py
There are six such fails.
@mwichmann Perhaps an additional dictionary mapping entry would work?
SCons\Tool\MSCommon\vc.py:
# Dict to 'canonalize' the arch
_ARCH_TO_CANONICAL = {
"amd64" : "amd64",
"emt64" : "amd64",
"i386" : "x86",
"i486" : "x86",
"i586" : "x86",
"i686" : "x86",
"ia64" : "ia64", # deprecated
"itanium" : "ia64", # deprecated
"x86" : "x86",
"x86_64" : "amd64",
"arm" : "arm",
"arm64" : "arm64",
"aarch64" : "arm64",
}
Assuming armv71 is a 32-bit architecture, the following might work:
"x86_64" : "amd64",
"arm" : "arm",
+ "armv71" : "arm",
"arm64" : "arm64",Observations:
- Forcing the platform to be win32 on non-win32 platforms seems like it is asking for trouble. The win32.py platform module does not raise any errors or warnings likely due entirely to the legacy test mindset described above. Judicious sanity checking for the win32 platform would then likely break any tests where the win32 platform is forced. For example there was a request in an earlier issue that SCons should produce a warning if COMSPEC is not valid. The COMSPEC value would not be "valid" on any non-win32 platforms.
- The GitHub test workflows have windows/msvc configuration(s). The windows/msvc test suite is run for all PR commits.
- The benefit of adding more error checking to the win32 platform and the mscommon far outweighs the benefit of running win32 tests on non-win32 platforms. Providing more checks in the win32 platform code and vc/vs environment is user-facing and likely could reduce reported bugs which is good for the project and good for users. Remotely debugging an end-user's environment might be easier if more configuration anomalies were detected and reported.
Observations:
* Forcing the platform to be win32 on non-win32 platforms seems like it is asking for trouble.
Indeed. I just saw that in the tests, too. Seems to me like it's one thing to run a test that doesn't need a backend system program to make sure the flow of text and variables through SCons comes out right when that flow isn't system-specific, but another to actively pretend to be an operating system you're not.
@jcbrill - and you could disable that sanity testing for those cross platform enabled testing?
and you could disable that sanity testing for those cross platform enabled testing?
I suppose. Though likely at the expense of simplicity and likely littered across multiple files (e.g., mscommon/vc.py, mscommon/common.py, mscommon/vs.py, platform/win32.py).
It would be far more straightforward to assume the win32.py platform and tools are only loaded on win32.
I'm a little fuzzy on the need for the msvs tests to run on non-windows platforms. The windows tests will be run in the github workflow tests (windows builds). I'm guessing that anyone interested in changes to the msvs generation code or using the functionality tested would likely be on windows anyway.
If my regular expression (e.g., [^.]platform\s*[=]\s*["']win32) search is correct:
- 1 test file named for VS 6.0 (1998)
- 3 test files named for VS 7.0 (2002)
- 3 test files named for VS 7.1 (2003)
- vs-scc-files.py
- vs-scc-legacy-files.py
- TestSConsMSVS.py
It is hard to imagine that there would be a problem running these tests only on windows. Otherwise, it seems like the tail wagging the dog.
@jcbrill - in the past not all developers had access to MSVC. And we didn't always have a windows CI machine. So there was value in supporting any tests which didn't actually require a working MSVC compiler to be able to run on any platform.
I suppose. Though likely at the expense of simplicity and likely littered across multiple files (e.g., mscommon/vc.py, mscommon/common.py, mscommon/vs.py, platform/win32.py).
Seems like a reasonable implementation would have one function call (is platform sane()...) , and that call could be disable-able easily?
So there was value in supporting any tests which didn't actually require a working MSVC compiler to be able to run on any platform.
There may have been some utility in the past. At present, the utility appears to have diminished quite significantly.
Seems like a reasonable implementation would have one function call (is platform sane()...) , and that call could be disable-able easily?
Something along those lines would probably work.
It is bizarre that the win32 platform code is required to be platform agnostic.
Which other platforms explicitly test that the platform is sane?
So there was value in supporting any tests which didn't actually require a working MSVC compiler to be able to run on any platform.
There may have been some utility in the past. At present, the utility appears to have diminished quite significantly.
Seems like a reasonable implementation would have one function call (is platform sane()...) , and that call could be disable-able easily?
Something along those lines would probably work.
It is bizarre that the win32 platform code is required to be platform agnostic.
Which other platforms explicitly test that the platform is sane?
@mwichmann - did you mean to close this?
no.. how did that happen???
@mwichmann POP QUIZ!
Tests:
- /test/MSVS/vs-files.py
- /test/MSVS/vs-scc-files.py
- /test/MSVS/vs-scc-legacy-files.py
- /test/MSVS/vs-variant_dir.py
Use this logic
import TestSConsMSVS
for vc_version in TestSConsMSVS.get_tested_proj_file_vc_versions():
test = TestSConsMSVS.TestSConsMSVS()
...
test.pass_test()
Do you see the bug?
that it won't keep going after the first time through the loop?
looks like someone took one of the single-version tests and just wrapped it without thinking?
and if the pass_test is moved outside, it fails on the second time through the loop... fun.
Excellent. (I had to put in a print statement.)
It was introduced in PR #3409. Basically these four tests only run for VC 8.0
This fixes the loop issue:
import TestSConsMSVS
test = None
for vc_version in TestSConsMSVS.get_tested_proj_file_vc_versions():
test = TestSConsMSVS.TestSConsMSVS()
...
test.pass_test(condition=False)
if test:
test.pass_test()
All four tests fail somewhere at or before version 10.0.
I'm working on a solution. There are issues with the generated GUIDS and what is expected. I think there may be an omission in Tools/msvs.py but I'm not sure. May need some help in a future PR.
The generated content for these test hasn't been checked since approximately SCons 3.1.
Disclosure: There is a bug for 14.3 in TestSConsMSVS that was my doing. Since the test didn't actually run for 14.3, I was unaware a lookup would fail.
see, it's better to leave tests incomplete and not slow down the development process 🥇
see, it's better to leave tests incomplete and not slow down the development process :-)
preaching to the choir!
It seems kind of odd to put a dummy pass inside the loop. The framework code certainly supports what you do, but I don't really get the point. Usally it's just test-test-test and if we got to the end, we didn't quit by failing, so execute the test.pass_test()
It seems kind of odd to put a dummy pass inside the loop.
Yeah. There really is not a good reason. Originally, I didn't know if there was some cleanup that might be triggered due to the construction of the test instance inside the loop. Then later realized it probably did not matter.
Anyway, we can leave it a bit odd for the time being... I only looked quickly and didn't understand the fail in the test I looked at:
Traceback (most recent call last):
File "/home/mats/github/scons/test/MSVS/vs-files.py", line 63, in <module>
assert vcxproj[:len(expect)] == expect, test.diff_substr(expect, vcxproj)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Actual did not match expect at char 547:
Expect: 'ns && "/home/mats/.pyenv/versions/venv-system31'
Actual: 'ns && "$(PYTHON_ROOT)/python" -c "fro'
So it looks like PYTHON_ROOT didn't expand the second time through? Oh... it's set to something odd at the end of the first loop and not restored.
That oddity was peculiar to test/MSVS/vs-files.py. Okay, I'll stop now - for that case I propose this patch, which exposes the guid problem you've already mentioned at 10.0. Notice the filename changes at that point.
diff --git a/test/MSVS/vs-files.py b/test/MSVS/vs-files.py
index b330ce726..780c081cb 100644
--- a/test/MSVS/vs-files.py
+++ b/test/MSVS/vs-files.py
@@ -33,6 +33,9 @@ import os
import TestSConsMSVS
+# overall test instance just in case loop gets no versions
+test = TestSConsMSVS.TestSConsMSVS()
+
for vc_version in TestSConsMSVS.get_tested_proj_file_vc_versions():
test = TestSConsMSVS.TestSConsMSVS()
host_arch = test.get_vs_host_arch()
@@ -91,6 +94,7 @@ for vc_version in TestSConsMSVS.get_tested_proj_file_vc_versions():
# Test that running SCons with $PYTHON_ROOT in the environment
# changes the .vcxproj output as expected.
+ save_python = os.environ.get('PYTHON_ROOT')
os.environ['PYTHON_ROOT'] = 'xyzzy'
python = os.path.join('$(PYTHON_ROOT)', os.path.split(TestSConsMSVS.python)[1])
@@ -102,8 +106,13 @@ for vc_version in TestSConsMSVS.get_tested_proj_file_vc_versions():
python=python)
# don't compare the pickled data
assert vcxproj[:len(expect)] == expect, test.diff_substr(expect, vcxproj)
+ # restore original value
+ if save_python is None:
+ del os.environ['PYTHON_ROOT']
+ else:
+ os.environ['PYTHON_ROOT'] = save_python
- test.pass_test()
+test.pass_test()
# Local Variables:
# tab-width:4I just added at the end of the loop:
del os.environ['PYTHON_ROOT']
I don't think it is supposed to be defined for the initial calls of the loop.
probably sufficient.
I have a version which passes for the four tests listed when running under WSL. I need to check if any of the changes to TestSConsMSVS or Tool/msvs broke any other tests on WSL and windows.
I'm uncomfortable with one change in particular to Tool/msvs. Once the tests pass, I'll push a branch to my SCons repository. If it's easier for discussion, a PR can be issued.
I think I should construct an issue for this, since this wasn't the original topic. Then you can point any PR to it. (#4609)
probably sufficient.
Sorry for being flippant. I had already fixed that one with a one-liner before your post and was working on the GUID problem at the time. We can revisit if you think it to be necessary.
@mwichmann Do you have a list of all the MS tests that failed on armv7?
There may be an easy fix. Some of the MSVS test scripts explicitly set HOST_ARCH which is set to x86 for unknown/unsupported architectures.
I suspect that the scripts that are failing are not forcing HOST_ARCH in the sconstruct/sconscript environment invocations. Might be worth rolling into #4610 if straightforward. Not sure how to test though.
Guessing these are the six tests that fail:
- /test/MSVS/vs-7.0-scc-files.py
- /test/MSVS/vs-7.0-scc-legacy-files.py
- /test/MSVS/vs-7.1-scc-files.py
- /test/MSVS/vs-7.1-scc-legacy-files.py
- /test/MSVS/vs-scc-files.py
- /test/MSVS/vs-scc-legacy-files.py
#4610 should fix this issue. Might be worth a try on armv7.
Removing platform='win32' from the msvs tests scripts works on WSL/ubuntu and windows (i.e., the msvs tool generates code as before the change on non-windows platforms).
If 4610 works on armv7, it might be worth testing removing forcing the platform and trying on armv7 as well.
yes, it was those six. won't be able test for a good chunk of today. will swing back later.