windows/msvc/paths.props: Dont add variant path for mpy-cross build. #11717

jimmo · 2023-06-06T13:16:54Z

If you try and build mpy-cross.vcxproj from Visual Studio (tested both 2017 and 2022), it expected the PyVariant property to be set, and without it ends up with an invalid include path, which results in a double-backslash in the cl.exe command line.

Same goes for using msbuild. It's easy to add /p:PyVariant=standard there but it should not be necessary.

paths.props is shared by mpy-cross.vcxproj, but in the mpy-cross build, PyVariant will be not set (because common.props explicitly doesn't set it when building mpy-cross).

However, paths.props will attempt to default PyVariantDir to variants\$(PyVariant)\ which will end up as variants\\ and the double backslash breaks the cl.exe command line.

This appears to work, but I wonder if all uses of PyVariantDir should also now have a Condition added?

cc @stinos

stinos · 2023-06-06T13:26:21Z

which results in a double-backslash in the cl.exe command line

Is that an issue?

jimmo · 2023-06-06T14:32:48Z

which results in a double-backslash in the cl.exe command line

Is that an issue?

Here's the command line from msbuild:

ClCompile:
  C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.36.32532\bin\HostX86\x86\cl.exe /c /I"C:\Use
  rs\jimmu\Desktop\micropython-remove-umodule\mpy-cross\\" /I"C:\Users\jimmu\Desktop\micropython-remove-umodule\\" /I"C
  :\Users\jimmu\Desktop\micropython-remove-umodule\ports\windows\\" /I"C:\Users\jimmu\Desktop\micropython-remove-umodul
  e\mpy-cross\build\\" /I"C:\Users\jimmu\Desktop\micropython-remove-umodule\ports\windows\msvc" /I"C:\Users\jimmu\Deskt
  op\micropython-remove-umodule\ports\windows\variants\\\" /ZI /JMC /nologo /W1 /WX ...

Which results in

cl : command line  error D8038: invalid argument 'C:\Users\jimmu\Desktop\micropython-remove-umodule\ports\windows\varia
nts\" /ZI /JMC /nologo /W1 /WX ...

It should be fairly easy to repro.

stinos · 2023-06-08T09:03:59Z

Hmm I cannot reproduce it on a fresh clone and after updating to the same version of the build tools you seem to have so something must be different. Can you explain what exactly you're doing?

Tried in VS, in Powershell and in cmd. I noticed that all of your /I have double quotes though, whereas I only have it for the mpy-cross ones.

Anyway, thing is: most win APIs should simply ignore double slashes so I have no idea what is wrong in your case. For reference this is one of the things which work ok for me, using cmd:

start->Developer Command Prompt for VS 2022
> systeminfo | findstr /c:"OS "

OS Name:                   Microsoft Windows 10 Pro
OS Version:                10.0.19044 N/A Build 19044

make sure it knows how to find python.exe (for me set PATH=c:\tools\miniconda3;%PATH%)
> git clone git@github.com:micropython/micropython.git %TEMP%\mp
> msbuild %TEMP%\mp\mpy-cross\mpy-cross.vcxproj

Relevant pieces of output:

MSBuild version 17.6.3+07e294721 for .NET Framework
...
MakeQstrData:
  cl.exe /nologo /Ic:\temp\usertemp\mp\mpy-cross\ /Ic:\temp\usertemp\mp\ /Ic:\temp\usertemp\mp\ports\windows\ /Ic:\temp\usertemp
  \mp\mpy-cross\build\ /Ic:\temp\usertemp\mp\ports\windows\msvc /Ic:\temp\usertemp\mp\ports\windows\variants\\ /D_USE_MATH_DEFIN
  ES /D_CRT_SECURE_NO_WARNINGS /D_CRT_NONSTDC_NO_WARNINGS /D_MBCS /DNO_QSTR /DMICROPY_GCREGS_SETJMP /DNO_QSTR /E c:\temp\usertem
  p\mp\py\qstrdefs.h c:\temp\usertemp\mp\ports\unix\qstrdefsport.h > c:\temp\usertemp\mp\mpy-cross\build\genhdr\qstrdefs.preproc
  essed.h
...
ClCompile:
  C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.36.32532\bin\HostX86\x86\cl.exe /c /I"c:\temp\usertem
  p\mp\mpy-cross\\" /Ic:\temp\usertemp\mp\ /Ic:\temp\usertemp\mp\ports\windows\ /I"c:\temp\usertemp\mp\mpy-cross\build\\" /Ic:\t
  emp\usertemp\mp\ports\windows\msvc /Ic:\temp\usertemp\mp\ports\windows\variants\\ /I"C:\Users\stijn\projects\vcpkg\installed\x
  86-windows\include" /ZI /JMC /nologo /W1 /WX /diagnostics:column /sdl- /MP /Od /Oy- /D _USE_MATH_DEFINES /D _CRT_SECURE_NO_WAR
  NINGS /D _CRT_NONSTDC_NO_WARNINGS /D _MBCS /Gm- /RTC1 /MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /Fo"c:\temp\use
  rtemp\mp\mpy-cross\build\DebugWin32\obj\\" /Fd"c:\temp\usertemp\mp\mpy-cross\build\DebugWin32\obj\vc143.pdb" /external:W1 /Gd
  /TC /analyze- /FC /errorReport:queue c:\temp\usertemp\mp\py\argcheck.c
...

KubaO · 2023-06-27T15:49:37Z

I can't reproduce it either. It just built for me without any adjustments.

jimmo · 2023-07-03T07:00:15Z

I was testing something else on Windows and took another look at this. The problem only happens for me when I download the repo zip from GitHub. It works fine from a Git checkout. (!!!)

The only obvious thing I could think of that might be different was maybe line endings but that doesn't appear to be the case.

stinos · 2023-07-03T08:53:23Z

Ok, so the issue is somehow triggered when putting micropython in a path with a dash like your C:\Users\jimmu\Desktop\micropython-remove-umodule but not C:\Users\jimmu\Desktop\micropython.. Doesn't matter if it's from zip or git checkout.

I'd prefer the fix to be more literally what the commit subject says though: 'Dont add variant path for mpy-cross build.' by following the pattern of 'if building mpy-cross then do not consider any of the variant stuff' already present in paths.props:

diff --git a/ports/windows/msvc/common.props b/ports/windows/msvc/common.props
index 55c9c934f..da84004f7 100644
--- a/ports/windows/msvc/common.props
+++ b/ports/windows/msvc/common.props
@@ -19,6 +19,9 @@
     <PyFileCopyCookie>$(PyBuildDir)copycookie$(Configuration)$(Platform)</PyFileCopyCookie>
     <CharacterSet>MultiByte</CharacterSet>
   </PropertyGroup>
+  <PropertyGroup Condition="'$(PyBuildingMpyCross)' != 'True'">
+    <PyIncDirs>$(PyIncDirs);$(PyVariantDir)</PyIncDirs>
+  </PropertyGroup>
   <ItemDefinitionGroup>
     <ClCompile>
       <AdditionalIncludeDirectories>$(PyIncDirs);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
diff --git a/ports/windows/msvc/paths.props b/ports/windows/msvc/paths.props
index 767772a1a..1268471ce 100644
--- a/ports/windows/msvc/paths.props
+++ b/ports/windows/msvc/paths.props
@@ -31,8 +31,8 @@
     <PyVariantDir Condition="'$(PyVariantDir)' == ''">$(PyWinDir)variants\$(PyVariant)\</PyVariantDir>
     <PyTargetDir Condition="'$(PyTargetDir)' == ''">$(PyBuildDir)</PyTargetDir>

-    <!-- All include directories needed for uPy -->
-    <PyIncDirs>$(PyIncDirs);$(PyBaseDir);$(PyWinDir);$(PyBuildDir);$(PyWinDir)msvc;$(PyVariantDir)</PyIncDirs>
+    <!-- Common include directories needed for uPy -->
+    <PyIncDirs>$(PyIncDirs);$(PyBaseDir);$(PyWinDir);$(PyBuildDir);$(PyWinDir)msvc</PyIncDirs>

     <!-- Within PyBuildDir different subdirectories are used based on configuration and platform.
          By default these are chosen based on the Configuration and Platform properties, but

Could create a separate PR for this if you want but seems better to keep this contained into one issue.

This file is shared by mpy-cross.vcxproj, where PyVariant will be not set (because common.props explicitly doesn't set it when building mpy-cross). Update common.props to only add the variant path to PyIncDirs if not building mpy-cross. This work was funded through GitHub Sponsors. Signed-off-by: Jim Mussared <jim.mussared@gmail.com>

jimmo · 2023-07-03T12:41:50Z

@stinos Thank you! I have no idea why the dash causes this and how you figured that out...!

I've updated the PR with your suggested diff.

stinos · 2023-07-03T12:54:42Z

I have no idea why the dash causes this and how you figured that out...!

I'm sure there's a reason somewhere to be found but didn't go looking for it so also no idea. After making 100% sure the content of the 2 micropython directories was exactly the same, I figured it could only be external, so first thing which came to mind was the dash because the sole difference seemed to be the path (even though 'mpy-cross' itself also has a dash!).

jimmo force-pushed the windows-variant-prop branch from 8f4a6d5 to fc9a276 Compare July 3, 2023 12:40

dpgeorge added the port-windows label Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

windows/msvc/paths.props: Dont add variant path for mpy-cross build. #11717

windows/msvc/paths.props: Dont add variant path for mpy-cross build. #11717

jimmo commented Jun 6, 2023

Uh oh!

stinos commented Jun 6, 2023

Uh oh!

jimmo commented Jun 6, 2023

Uh oh!

stinos commented Jun 8, 2023 •

edited

Loading

Uh oh!

KubaO commented Jun 27, 2023

Uh oh!

jimmo commented Jul 3, 2023

Uh oh!

stinos commented Jul 3, 2023

Uh oh!

jimmo commented Jul 3, 2023

Uh oh!

stinos commented Jul 3, 2023

Uh oh!

Uh oh!

Uh oh!

windows/msvc/paths.props: Dont add variant path for mpy-cross build. #11717

Are you sure you want to change the base?

windows/msvc/paths.props: Dont add variant path for mpy-cross build. #11717

Conversation

jimmo commented Jun 6, 2023

Uh oh!

stinos commented Jun 6, 2023

Uh oh!

jimmo commented Jun 6, 2023

Uh oh!

stinos commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KubaO commented Jun 27, 2023

Uh oh!

jimmo commented Jul 3, 2023

Uh oh!

stinos commented Jul 3, 2023

Uh oh!

jimmo commented Jul 3, 2023

Uh oh!

stinos commented Jul 3, 2023

Uh oh!

Uh oh!

stinos commented Jun 8, 2023 •

edited

Loading