BUG: Use meson compile wrapper on Windows #371

lithomas1 · 2023-03-29T14:27:02Z

Necessary if e.g. the user passes requests vsenv from meson setup.
(I guess I could restrict this to only when vsenv is requested in setup, but this needs thought as to how it would work with existing builddirs. I don't remember OTOH if we reconfigure the build dir if it exists already).

With this, pandas should be getting close to greenish again.

I still need to add a test for this, but this should be the right idea.

rgommers · 2023-03-29T14:51:51Z

Ah yes, now that I see this, I am relying on --vsenv too, and it seems like we switched from using meson compile to ninja sometime after the 0.12.0 release. In PyWavelets/pywt#667 I have:

python.exe -m pip install . -v --config-settings=setup-args=--vsenv

and that worked quite nicely.

dnicolodi · 2023-03-29T14:57:31Z

Isn't there anywhere a command that can be used like env that populates the required environment variables to use MSVC and calls its argument, like vsenv python.exe -m pip install . -v? That would be the most obvious solution, and a better UX IMHO, the --config-settings=setup-args=--vsenv is not really that obvious or discoverable.

If it does not exist, someone should write it.

lithomas1 · 2023-03-29T15:05:09Z

Isn't there anywhere a command that can be used like env that populates the required environment variables to use MSVC and calls its argument, like vsenv python.exe -m pip install . -v? That would be the most obvious solution, and a better UX IMHO, the --config-settings=setup-args=--vsenv is not really that obvious or discoverable.

You would really only use the --vsenv feature in CI(so the UX doesn't matter that much) because typically you wouldn't have the messed up situation with different compilers being/not being on the PATH that Github Actions does.

Also I would argue, having to hardcode the path to vcvarsall.bat or add another dependency to do this for me, is worse than adding just--config-settings=setup-args=--vsenv.

lithomas1 · 2023-03-29T15:06:32Z

Isn't there anywhere a command that can be used like env that populates the required environment variables to use MSVC and calls its argument, like vsenv python.exe -m pip install . -v? That would be the most obvious solution, and a better UX IMHO, the --config-settings=setup-args=--vsenv is not really that obvious or discoverable.

If it does not exist, someone should write it.

Right now the easisest way to do this is
call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Auxiliary\Build\vcvarsall.bat"

which doesn't work on bash and also not on powershell.

rgommers · 2023-03-29T15:10:40Z

I agree with @lithomas1 on the UX point, the --config-settings way (which will soon be -C) is a lot nicer than the MSVC-native stuff with .bat script. The vcvarsall.bat paths are changing regularly and there aren't any guarantees for where it lives. This is what works in GHA/bash, and it makes me a bit sad:

          function Invoke-VSDevEnvironment {
            $vswhere = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe"
              $installationPath = & $vswhere -prerelease -legacy -latest -property installationPath
              $Command = Join-Path $installationPath "Common7\Tools\vsdevcmd.bat"
            & "${env:COMSPEC}" /s /c "`"$Command`" -arch=amd64 -no_logo && set" | Foreach-Object {
                  if ($_ -match '^([^=]+)=(.*)') {
                      [System.Environment]::SetEnvironmentVariable($matches[1], $matches[2])
                  }
              }
          }
          Invoke-VSDevEnvironment

dnicolodi · 2023-03-29T15:18:05Z

There are good reasons why we moved from meson compile to directly calling ninja. Recommendation from the Meson maintainers is one. I would carefully consider other options before reverting that.

IIUC, this PR is relevant only for CI setups where there are multiple compilers installed. For my projects I use an action to setup the required environment variables. Therefore, the problem is limited to CI setups with multiple compilers and using some shell where the action does not work. Would an action that works for setting up the environment variables for an UNIX-like shell be an acceptable solution?

rgommers · 2023-03-29T15:26:35Z

I'm actually using it in Appveyor, so a GHA action isn't helpful. That said, I haven't looked under the hood to see what --vsenv does and if there's something sensible to replace it with. Ideally we find something nicer than cargo culting the above bash function.

Perhaps adding native support in meson-python for a --use-msvc <optional year/version> flag or some such thing?

lithomas1 · 2023-03-29T15:28:09Z

I would still prefer sticking to meson. IIUC, the -vsenv option was created to be used CI environments, and does something very similar to what @rgommers mentioned above.
(See https://github.com/mesonbuild/meson/blob/b30cd5d2d587546eac8b560a8c311a52d69fb53e/mesonbuild/utils/vsenv.py#L54-L70)

I'm not against using an action, but meson's solution is just better, since it is CI-provider agnostic, and also should be shell agnostic.

I think the only argument made against it was
#328 (comment), where it is mentioned to be pretty slow. (The solution would be to just use the wrapper where necessary, i.e. -vsenv is passed into our setup options)

dnicolodi · 2023-03-29T15:41:58Z

If the issue presents itself almost exclusively in CI setups, I don't understand the reason for the reluctance in using a simple external tool to solve the issue in a flexible way. Taking the implementation of setup_vsenv() from Meson and trowing it into a stand-alone script that looks up the required environment variables and executes os.execvpe(sys.argv[1], sys.argv[1:], env) seems a very good solution to me. And it just requires a pip install vsenv or something similar.

dnicolodi · 2023-03-29T19:39:47Z

Here it is https://github.com/dnicolodi/vsenv

No documentation and no tests, but it should be doing exactly what I was describing above.

rgommers · 2023-03-30T10:14:24Z

If the issue presents itself almost exclusively in CI setups,

I don't think it does? Anyone wanting to use MSVC on Windows may run into this. The Visual Studio activation is run on every subcommand say the docs. The downside of that is timing (from gh-328: meson compile, 1 second. ninja, 0.005 seconds), the upside is that things work.

For non-editable builds, the 1 sec. gain is not interesting compared to the UX gains of things not mysteriously breaking when you, for example, run dev commands in a freshly opened Git Bash shell. However, I agree that for editable builds where ninja gets invoked all the time, the extra 1 sec. is annoying. Perhaps we can get the best of both worlds here by documenting that if folks want to use MSVC on Windows together with editable builds, they should do separate env activation in their shell themselves - and we give them a tool for that like the vsenv repo above. WDYT?

dnicolodi · 2023-03-30T11:34:22Z

The fact that the issue is mostly encountered in CI context was reported as a justification for not having an optimal UX. I just repeated the statement. If this is something that users need to use, the --config-settings=setup-args=--vsenv incantation seems way too magical. Having different compilation commands depending on the platform, on which options are provided by the user, or on whether a regular wheel or an editable wheel is being built, is also too magical, too complex, fragile, and potentially source of inconsistencies.

Documenting that the MSVC compiler needs to be in the PATH (when other compiler suites are found on the PATH) is the correct solution. I already opened a bug about this a while ago, see #224.

Note that the vsenv tool I hacked together does not set the environment for the current shell. It is a wrapper that sets up the required environment variables and invokes the command specified as an argument, like env does.

I'm really not that keen to compromises motivated to fixup basic compilation environment setup requirements. If you want to run a compiler, the compiler needs to be found on the PATH. If your compiler vendor makes it ridiculously hard to have that setup properly, please go complain to your compiler or OS vendor.

rgommers · 2023-03-30T11:57:12Z

If your compiler vendor makes it ridiculously hard to have that setup properly, please go complain to your compiler or OS vendor.

I don't think that's a reasonable stance. I also very much dislike how complex it is to use MSVC from the command line, and that MSVC is not on the PATH by default when you install it, but that won't be changed by Microsoft because we don't like it. So we should try our best to make it work. And it already works with plain Meson out of the box when MSVC is the only installed compiler - we're just actively avoiding that as a performance optimization by calling ninja directly. This was simply missed in the discussion when we moved from meson compile to ninja it seems. So we should undo that change if and only if it's needed - getting compiler-not-found errors like this is really a time sink and frustrating for the average user.

UX-wise, -Csetup-args=--vsenv is perhaps as easy as it gets in the "other compilers were found on PATH" case, unless we want to add a separate --use-msvc option to meson-python directly.

We still may have an issue then (not actually clear to me), which is that it doesn't work out of the box without that argument. Which I think Meson itself does seem to handle. So using meson compile on Windows unconditionally addresses that, but then we're also using it when we don't need it. I don't think we could introspect the result of meson setup for this, could we?

I don't have a Windows machine at hand, it'd be great to be able to check if for regular usage without --vsenv things continue working or we have a regression compared to 0.12.0. If one only has MSVC and calls meson setup build && ninja -C build, does that work or fail? And if it fails, does meson setup build && meson compile -C build instead actually work?

dnicolodi · 2023-03-30T12:33:31Z

Sorry, I misunderstood. I thought that the issue was only present with more than one compiler installer on the system. However, obviously, the issue is also when only MSVS is present as there is nothing that setups the environment also in this case when meson-python invoked ninja. To answer your question: this is indeed a regression compared to meson-python 0.12.0 as calling meson setup build && ninja -C build does not find the compiler, meson setup build && meson compile -C build works.

I still think that having the compilers in the PATH is a more than reasonable solution, and that vendors should be pressured to provide a solution to easily do that. However, indeed, I don't see this happening any time soon. It would be great if Windows and Visual Studio users would make their license money worth and demand things to work reasonably, but I have the impression that open source maintainers are easier to reach that the Microsoft technical support.

Let's revert back to use meson compile for compilation. Should we do this always or only on Windows? Have everyone pay the price of longer execution times sucks, but I really don't like having different behaviors for different systems. It is also a pain to document: every time we mention the compilation command we need to say that it is different on Windows and on other systems.

rgommers · 2023-03-30T14:34:03Z

Have everyone pay the price of longer execution times sucks, but I really don't like having different behaviors for different systems.

Yeah, there's a tradeoff between performance and complexity here. I don't have a clear preference - I think I agree with what you propose here, but I could see us going another way later perhaps. The third option is to default to meson compile everywhere, and let users opt in to skipping that and calling ninja instead. Also extra complexity, so may only worth doing later if it turns out to be a pain point?

One other thing I'd like to do is add a test with code that is MSVC-specific and only runs on Windows with MSVC installed. That way we're dogfooding whatever we do here.

dnicolodi · 2023-03-30T14:56:22Z

I'm not sure I proposed a solution, I think I have more questions than answers on this issue. IMHO the reasonable options are:

always use meson compile
use meson compile on Windows and ninja everywhere else
use meson compile and allow the user to opt in to use ninja

I think 2. is the best option. The execution time penalty for editable installations on systems other than Windows is too large to pay, and having a different compilation command for editable and regular builds is much more surprising than a difference based on the platform. Having an user option for this seems a lot of work for very little gain.

I'll polish @lithomas1 patch and add a documentation patch on top.

rgommers · 2023-03-30T15:08:56Z

Ah okay, I misunderstood what you meant. Yes, I think (2) seems like the way to go at the moment, +1 for that.

lithomas1 · 2023-03-30T16:35:22Z

I'm not sure I proposed a solution, I think I have more questions than answers on this issue. IMHO the reasonable options are:

always use meson compile

use meson compile on Windows and ninja everywhere else

use meson compile and allow the user to opt in to use ninja

I think 2. is the best option. The execution time penalty for editable installations on systems other than Windows is too large to pay, and having a different compilation command for editable and regular builds is much more surprising than a difference based on the platform. Having an user option for this seems a lot of work for very little gain.

I'll polish @lithomas1 patch and add a documentation patch on top.

@dnicolodi You should be able to push to my branch directly if you want (I ticked the box allowing maintainers to push).
I'd be in favor of 2 as well.

dnicolodi · 2023-03-30T18:14:29Z

Another possible solution came to mind:

integrate code similar to vsenv linked above into meson-python and use it to setup the environment before calling ninja.

Drawbacks of this approach are that this is a little bit more code that needs to be maintained in meson-python and the code would have to de duplicated in every editable package loader, although a hundred lines of code do not really seem to be that significant in the broad picture.

Advantages are that we don't need to pay the meson compile startup time penalty on any platform and that we can easily offer a bit nicer python -m pip install --config-setting=vsenv command line interface.

rgommers · 2023-03-30T19:11:09Z

Maybe we can start with (2) at least, and consider later if (4) is needed? Alternatively, we could help with optimizing meson compile import times and also with ensuring --vsenv is as robust as it can be (IIRC it has issues in GitHub Actions and those need fixing up). More work probably, but avoids code duplication and benefits more Meson users.

FFY00 · 2023-03-30T20:26:32Z

FYI I am +1 of bringing back meson compile. IMO the points against it weren't that strong, and it is a breaking change.

Also, IMO having meson compile instead of calling ninja diretcly is better for backwards compatibility.

Recommendation from the Meson maintainers is one. I would carefully consider other options before reverting that.

Can you link to those recommendations? It might be relevant when deciding this. I honestly don't really remember anything major, just that it doesn't actually invoke other build backends on itself.

I do not wish to argue about this, so please just take my opinion as-is and refer to the other maintainers to make the final call.

rgommers · 2023-03-30T20:29:40Z

@FFY00 we're already on the same page about this one - we all agree now it's a breaking change and we'll fix it before the release.

FFY00 · 2023-03-31T09:20:26Z

Okay, so, here's my opinion. Let's roll back the meson compile > ninja change globally, and open a new issue to discuss strategies to possibly get rid of the meson compile penalty on editable installs.

@lithomas1 could you update the PR to use meson compile all the time, instead keeping ninja and only selecting meson compile on Windows?

This is a release blocker, so I'd like to get this solved soon, so that we can finally have a stable release with editable installs support. And thank you very much for reporting the issue and opening the PR!

FFY00 · 2023-03-31T09:30:01Z

I opened #373 to track the editable installs speedup. Let's release with meson compile and then look at that and see what we can do to improve it more calmly.

dnicolodi · 2023-03-31T09:30:17Z

@rgommers @lithomas1 and I already agreed on the way forward. Why is that decision being disregarded?

dnicolodi · 2023-04-02T21:15:36Z

Indeed. The funny thing is that it seem to depend on the optimization level: that should be the only difference between using -Ddebug=false and -Ddebug=false -Dbuildtype=plain.

dnicolodi · 2023-04-02T21:28:59Z

One more data point: -Ddebug=false -Doptimization=plain still results in the link failure.

dnicolodi · 2023-04-02T22:09:26Z

Either I've been staring at this too long or there is something unusual going on: the link command that fails and the one that succeeds are identical.

dnicolodi · 2023-04-02T22:49:24Z

Found it! The issue does not depend on how the Visual Studio environment gets activated. It really depends on the buildtype option. I think there is still some confusion in the interplay of the debug, optimization, and buildtype options on the Meson side. The issue is caused by the compiler options used for -Dbuildtype=debug and for -Dbuildtype=plain. The fact that the compiler options cause a linker error made the failure look magical.

After removing the common options, the default -Dbuildtype=debug uses these compiler options: "/MDd" "/O2", -Dbuildtype=plain uses instead: "-DNDEBUG".

Clearly the "/MDd" option is the issue. I'm not entirely sure whether this behavior is correct or not.

eli-schwartz · 2023-04-02T22:58:23Z

From https://mesonbuild.com/Builtin-options.html#base-options

The default value of b_vscrt is from_buildtype.

That's what sets /MDd.

dnicolodi · 2023-04-02T23:05:09Z

Thanks, I was not aware of that option. Based on that, I would say that Meson is doing what it is asked to do. I was a bit surprised that setting -Ddebug=false the build type is still debug, but I also don't see why it should not if it is not changed explicitly.

For meson-python, I really don't know what the best way forward is. It seems that, if we set -Ddebug=false we should not be keeping the build type to be debug, but I also don't know what we should change it to. Changing the build type seems like a pretty heavy hammer that is going to override what developers have put in their meson.build. Maybe we should simply change b_vscrt to something else, but I don't know what the right choice would be. I don't know much about compilation on Windows and what changing this really does.

dnicolodi · 2023-04-02T23:27:32Z

One thing that I don't understand is that buildtype=plain does not specify any runt time library flag. From reading about it here https://learn.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library?view=msvc-170 I would expect that at least one of that flags needs to be specified.

dnicolodi · 2023-04-02T23:54:50Z

It is also surprising that buildtype=debugoptimized does not use the debug version of the runtime library
https://github.com/mesonbuild/meson/blob/71487c876e6f0046cbb09fce23b09d8a38a8224f/mesonbuild/compilers/mixins/visualstudio.py#L381-L382

dnicolodi · 2023-04-08T21:54:30Z

With #383 merged, this can be merged without the workaround for the linking problem.

dnicolodi · 2023-04-10T12:36:39Z

Rebased once more. @rgommers is there any reason for not merging this?

rgommers · 2023-04-10T13:34:28Z

Rebased once more. @rgommers is there any reason for not merging this?

I don't think so - I'll make time for a final review(/merge) later today.

rgommers

The tests pass and this all looks quite good to me. Just a few very minor things I noticed (docs, typos). @dnicolodi please have a look and feel free to merge when those are addressed or if I'm wrong about them.

tests/test_project.py

rgommers · 2023-04-10T21:46:37Z

tests/test_project.py

+            # pass them to the --ninja-args option
+            assert cmd[-1].startswith('--ninja-args=')
+            cmds.append(cmd[:2])
+            args.append(ast.literal_eval(cmd[-1].split('=')[1]))


slightly painful I guess, but fine in a test case

What are you referring to? The not-so-nice part is marshaling the arguments into the format expected by Meson. Parsing them back is just a consequence of that. However, the code for either does not look that horrible.

tests/test_project.py

rgommers · 2023-04-10T21:49:33Z

docs/how-to-guides/meson-args.rst

@@ -44,6 +44,20 @@ building a Python wheel. User options specified via ``pyproject.toml``
 or via Python build front-end config settings override the
 ``meson-python`` defaults.

+When building on Windows, ``meson-python`` invokes the ``ninja``
+command via the ``meson compile`` wrapper. When the GCC or the LLVM


Is it actually only GCC/LLVM, or any other compiler? I'd expect the latter. If so, maybe rephrase like "When no other compiler known to Meson (e.g., GCC or Clang-cl) is found on ...."

The same info is repeated under Examples, so if editing here then it needs editing further down too.

In terms of the code implementation, we quite literally check for shutil.which with any of cc, gcc, clang, clang-cl. The remaining autodetected default compilers for C are icl and pgcc, but icl ships with its own cl.exe (super "helpful" as noted by the code comments in detect.py) and meson will prefer cl.exe over pgcc anyway, if it can.

The --vsenv code is not kept in sync with detect.py's list of default compiler search names, whether it "should" do so is another question (that I don't have an opinion on, to be clear. :D)

I haven't found detailed documentation for the compiler auto detection. I had a look at the code, which behaves as @eli-schwartz describes https://github.com/mesonbuild/meson/blob/eb472a133f45826a246b40c3d775eebf098fd6b1/mesonbuild/utils/vsenv.py#L43-L51 I don't know how much of this is appropriate to document on the meson-python side, given that it is not explicitly documented and that I don't know how much of it is susceptible of change in Meson. I thought that GCC and LLVM covered the most popular cased and anyone using a less common compiler would know what they are doing.

Thanks for the explantions both - looks like we're good then with the current docs.

I don't know how much of this is appropriate to document on the meson-python side, given that it is not explicitly documented and that I don't know how much of it is susceptible of change in Meson.

agreed

tests/packages/detect-compiler/detect_compiler.c

The meson compile wrapper is required on Windows to setup the MSVC compiler environment. Using the --ninja-args option to meson compile allows to provide the exact same semantics for the compile arguments. The format in which ninja command line arguments need to be passed to --ninja-args makes the interface a bit awkward to use and adds some complexity to this otherwise simple patch.

lithomas1 added the bug Something isn't working label Mar 29, 2023

FFY00 added the release-blocker label Mar 31, 2023

FFY00 added this to the v0.13.0 milestone Mar 31, 2023

FFY00 mentioned this pull request Mar 31, 2023

0.13.0 release #372

Closed

FFY00 mentioned this pull request Mar 31, 2023

Speeding up editable installs #373

Closed

dnicolodi force-pushed the fix-clexe-path branch from a4d29cb to 338ede8 Compare April 2, 2023 21:44

dnicolodi force-pushed the fix-clexe-path branch from e924405 to a3abba0 Compare April 2, 2023 22:16

dnicolodi force-pushed the fix-clexe-path branch from a3abba0 to 30fda4b Compare April 2, 2023 23:51

dnicolodi force-pushed the fix-clexe-path branch 3 times, most recently from 4ebb5b5 to d49aa11 Compare April 5, 2023 07:33

dnicolodi mentioned this pull request Apr 5, 2023

Defaults for Meson build options, linking issues with MSVC #381

Closed

dnicolodi force-pushed the fix-clexe-path branch 3 times, most recently from fe1eb11 to 6848361 Compare April 8, 2023 21:44

dnicolodi force-pushed the fix-clexe-path branch 2 times, most recently from 418d71c to 5f96c46 Compare April 10, 2023 12:27

rgommers approved these changes Apr 10, 2023

View reviewed changes

lithomas1 and others added 2 commits April 11, 2023 00:43

TST: verify that compilation uses MSVC when available

cd81e7b

dnicolodi force-pushed the fix-clexe-path branch from 5f96c46 to cd81e7b Compare April 10, 2023 22:43

dnicolodi merged commit e742188 into mesonbuild:main Apr 10, 2023

lithomas1 deleted the fix-clexe-path branch April 10, 2023 23:00

rgommers mentioned this pull request Apr 14, 2023

Document that Meson and thus meson-python require MSVC compilers to be found in $PATH #224

Closed

BUG: Use meson compile wrapper on Windows #371

BUG: Use meson compile wrapper on Windows #371

Conversation

lithomas1 commented Mar 29, 2023

rgommers commented Mar 29, 2023

dnicolodi commented Mar 29, 2023 • edited Loading

lithomas1 commented Mar 29, 2023

lithomas1 commented Mar 29, 2023

rgommers commented Mar 29, 2023

dnicolodi commented Mar 29, 2023

rgommers commented Mar 29, 2023

lithomas1 commented Mar 29, 2023

dnicolodi commented Mar 29, 2023

dnicolodi commented Mar 29, 2023 • edited Loading

rgommers commented Mar 30, 2023

dnicolodi commented Mar 30, 2023

rgommers commented Mar 30, 2023

dnicolodi commented Mar 30, 2023

rgommers commented Mar 30, 2023

dnicolodi commented Mar 30, 2023 • edited Loading

rgommers commented Mar 30, 2023

lithomas1 commented Mar 30, 2023

dnicolodi commented Mar 30, 2023 • edited Loading

rgommers commented Mar 30, 2023

FFY00 commented Mar 30, 2023 • edited Loading

rgommers commented Mar 30, 2023

FFY00 commented Mar 31, 2023

FFY00 commented Mar 31, 2023

dnicolodi commented Mar 31, 2023

dnicolodi commented Apr 2, 2023

dnicolodi commented Apr 2, 2023

dnicolodi commented Apr 2, 2023

dnicolodi commented Apr 2, 2023

eli-schwartz commented Apr 2, 2023

dnicolodi commented Apr 2, 2023 • edited Loading

dnicolodi commented Apr 2, 2023

dnicolodi commented Apr 2, 2023

dnicolodi commented Apr 8, 2023

dnicolodi commented Apr 10, 2023

rgommers commented Apr 10, 2023

rgommers left a comment

Choose a reason for hiding this comment

rgommers Apr 10, 2023

Choose a reason for hiding this comment

dnicolodi Apr 10, 2023

Choose a reason for hiding this comment

rgommers Apr 10, 2023

Choose a reason for hiding this comment

rgommers Apr 10, 2023

Choose a reason for hiding this comment

eli-schwartz Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

dnicolodi Apr 10, 2023

Choose a reason for hiding this comment

rgommers Apr 10, 2023

Choose a reason for hiding this comment

dnicolodi commented Mar 29, 2023 •

edited

Loading

dnicolodi commented Mar 29, 2023 •

edited

Loading

dnicolodi commented Mar 30, 2023 •

edited

Loading

dnicolodi commented Mar 30, 2023 •

edited

Loading

FFY00 commented Mar 30, 2023 •

edited

Loading

dnicolodi commented Apr 2, 2023 •

edited

Loading

eli-schwartz Apr 10, 2023 •

edited

Loading