Page MenuHomePhabricator

[libcxx] [docs] Update docs about how to build for Windows
ClosedPublic

Authored by mstorsjo on Feb 21 2021, 3:28 PM.

Details

Summary

Refresh the existing paragraphs on building in MSVC configurations, add a sample of one working configuration for MinGW, and add more details on what's necessary to run the tests these days.

While I know quite a few people and organizations do build libcxx for Windows (adding a couple of them as reviewers here), not all of them maybe do build it with CMake or run the tests (via the default llvm-lit driver) - I'm trying to document some common ground here, to get some reference config for proceeding with fixing the state of the tests.

Diff Detail

Event Timeline

mstorsjo created this revision.Feb 21 2021, 3:28 PM
mstorsjo requested review of this revision.Feb 21 2021, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2021, 3:28 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
rnk added inline comments.Feb 22 2021, 11:24 AM
libcxx/docs/BuildingLibcxx.rst
76–78

I would update this to "VS 2017 or newer (19.14) is required", to be in line with the cmake checks in LLVM, which look for _MSC_VER 19.14.

88–89

I guess this becomes "Visual Studio 15 2017"

89–90

I'm not sure what this should be. =/

Thanks for having a look!

libcxx/docs/BuildingLibcxx.rst
76–78

I guess so, yeah, this is a bit old...

89–90

I'll see if I can try actually running this config - I don't really see it as something that is used much, but I wouldn't want to remove it either.

111

@rnk - Does this match how you generally build/run tests for other llvm components these days, or do you use some other provider of a unix shell, or do the rest of the tests cope with just a plain cmd shell?

At least for the libcxx tests, running tests in a cmd shell errors out like this:

Script:
--
echo 'COMPILED WITH' > nul &&  'C:/Program Files/LLVM/bin\clang++.EXE' ...

--
Exit Code: 9009

Command Output (stderr):
--
''C:' is not recognized as an internal or external command,
operable program or batch file.
rnk accepted this revision.Feb 22 2021, 12:49 PM

lgtm in any case, this is an improvement.

libcxx/docs/BuildingLibcxx.rst
111

Yes, this is pretty close to what I do. I usually develop in bash, and then bash is on PATH, and most lit tests succeed that way. The codepath where lit invokes cmd to run tests is not maintained, and should ideally be removed.

However, you are probably aware that all of the other lit test suites in LLVM avoid the use of a shell altogether, instead using the lit internal shell. At the time, skipping the extra shell subprocess in every test saved a lot of time for me, so I put a fair amount of effort into migrating to the internal shell. It might be worth doing the same for libc++, but I haven't looked into it at all.

compnerd added inline comments.Feb 22 2021, 2:26 PM
libcxx/docs/BuildingLibcxx.rst
104

I believe that the documentation suggestions GNUWin32 for the tools, it would be nice to suggest that for consistency.

110

I think it would be better to just use path %PATH%;%ProgramFiles%\Git\usr\bin as git is also provided by MSVC, and is going to be needed to clone the projects, so it avoids an additional dependency.

131

This would not be needed with the suggestion above right?

mstorsjo added inline comments.Feb 22 2021, 2:27 PM
libcxx/docs/BuildingLibcxx.rst
111

Thanks for confirming!

Yeah there's certainly lots to improve in running the libcxx tests on windows too, but the major issue is getting the tests back into a working shape overall - right now there's at least two that more or less block all tests from functioning, the npos issue, and D97167. And overall getting it into a more reproducible setup where more than one individual can plausibly run them...

mstorsjo added inline comments.Feb 22 2021, 2:34 PM
libcxx/docs/BuildingLibcxx.rst
104

GNUWin32 doesn't supply e.g. bash, and afaik @rnk and others have moved away from setups using only that (and that it isn't enough here). In particular here, running things in a plain cmd shell doesn't work - right now. (It might work after more fixes somewhere, but I'm trying to get the shortest path towards a documented working configuration.)

110

Hmm, maybe, that's worth a try. That would at least provide bash, but it remains to see if it hooks that up the right way, with all the interactions between python/llvm-lit and everything.

131

If not using a MSYS2 shell, then no, this one wouldn't be needed no.

mstorsjo added inline comments.Feb 23 2021, 11:50 AM
libcxx/docs/BuildingLibcxx.rst
110

That kinda seems to work, but breaks for other reasons. Some later bit tries to execute the python interpreter, and falls down due to incorrect quoting of parts of C:\Program Files (x86)\Python...

So while it'd be nice, I'll stick with documenting this configuration that works right now (at least after landing D97167, D97168 and D97294).

I'll update the patch in any case with a bit more clarifications to help others reproduce the setup, and the other suggestions handled.

mstorsjo added inline comments.Feb 23 2021, 2:07 PM
libcxx/docs/BuildingLibcxx.rst
110

Actually, it does work with a python installed to a path that doesn't contain spaces (e.g. C:\Users\Administrator\AppData\Local\Programs\Python\Python39 works, but C:\Program Files\Python.. doesn't).

I'll try to mention both approaches in the doc.

mstorsjo added inline comments.Feb 23 2021, 2:32 PM
libcxx/docs/BuildingLibcxx.rst
89–90

@compnerd - WDYT about the section in generating VS project files at this point - is it worth keeping (with updated version numbers) - is it something people do, or should we scrap it (as I'm adding a couple other configs here anyway)?

mstorsjo added inline comments.Feb 24 2021, 12:39 AM
libcxx/docs/BuildingLibcxx.rst
89–90

Ok, with VS 2017, with a now discontinued VS extension, this should be -T LLVM (not tested myself). With VS 2019, with integration shipped as part of Visual Studio itself, this parameter should be -T ClangCL. I think I'll update the doc with the 2019 approach.

mstorsjo updated this revision to Diff 326140.Feb 24 2021, 10:26 AM

Updated the MSVC IDE cmake setup and version requirements, made msys optional for the msvc testing setup, fixed running the tests in mingw configs (depending on D97294).

compnerd added inline comments.Mar 1 2021, 1:23 PM
libcxx/docs/BuildingLibcxx.rst
89–90

Im not too worried about the VS path really. It would be nice to keep it, but if we cannot, we can remove it with a note saying that we would be accepting of patch to get MSVC as an IDE working. Personally, I just use CMake + Ninja as the builder and use vim for editing.

104

I really would like to keep the cmd environment support. I do use that environment for a downstream project.

110

Actually, the path with spaces is good to test - we should identify where it breaks, it sounds like a bad lit substitution (missing quotes).

mstorsjo added inline comments.Mar 1 2021, 1:31 PM
libcxx/docs/BuildingLibcxx.rst
89–90

I kept this section for now by updating it to a config that I could test that seemed to work, but as you say, it's not a config I'd expect to be used much in practice.

110

Yeah I fixed that one in D97369 (now merged), so it does run fine in plain cmd now - the current version of this patch now showcases how to do it by adding the Git for Windows binaries to the path, but also mentions how to do it with MSYS2 and mentions the quirks one could run into there. Please have a look at the latest version of this patch.

compnerd accepted this revision.Mar 2 2021, 8:43 AM

Thanks for the updates!

curdeius accepted this revision as: curdeius.Mar 12 2021, 7:37 AM

LGTM.

Quuxplusone added inline comments.
libcxx/docs/BuildingLibcxx.rst
104

/shell, by/shell by/

115

/case, in the right environment, run:/case, then run:/
I assume "in the right environment" means simply "following right after the environment-setup step that we just explained in the previous paragraph." There's no need to scare the reader with the implication that their environment could at this point still be wrong in some unspecified way.

123–124

Why forward-slashes on line 123 and backslashes on line 125? I think they should be forward-slashes in both places, if that's possible (i.e. if it'd do the right thing). And if the backslashes are important for correctness, that should probably be explained here.

127–131
148

remove blank line

libcxx/www/index.html
133–135
mstorsjo marked 4 inline comments as done.Mar 12 2021, 9:54 AM
mstorsjo added inline comments.
libcxx/docs/BuildingLibcxx.rst
123–124

They're interpreted kinda differently - there's a lot of vague differences between if you run things in a bash shell, a cmd shell, or you pass paths that are interpreted by cmake at a later point. But it looks like this path works with forward slashes. And it turns out that -DLLVM_PATH actually is entirely unnecessary at this point, will remove that one.

127–131

Thanks for the rewording.

One tweak though; adding that LIBCXX_TARGET_TRIPLE doesn't make it call clang-cl instead of clang++, but it adds -target x86_64-windows-msvc to the clang++ command line.

What about "... you should tell check-cxx to make clang++ use the right target by adding -DLIBCXX_TARGET_TRIPLE=x86_64-windows-msvc to the cmake command line above."?

mstorsjo updated this revision to Diff 330287.Mar 12 2021, 9:54 AM

Incorporated the suggestions

@Quuxplusone Does this look acceptable to you now?

Quuxplusone accepted this revision.Mar 14 2021, 7:40 PM

LGTM, modulo one more re-rewrite. Again, I don't know much about Windows, but more documentation ought to always be a good thing. :)

libcxx/docs/BuildingLibcxx.rst
127–131

Ah, I see. I'm not thrilled by the structure "You should tell X to do Y by Z'ing" because it's ambiguous whether my Z'ing will make X do Y, or whether I'm supposed to do something unspecified in order to make X do-Y-by-Z'ing. Therefore I suggest the following re-rewrite:

If you have installed the MSYS2-provided clang package (which defaults
to a non-MSVC target), you should add -DLIBCXX_TARGET_TRIPLE=x86_64-windows-msvc
to the cmake command line above. This will instruct check-cxx to use
the right target triple when invoking clang++.

This revision is now accepted and ready to land.Mar 14 2021, 7:40 PM
mstorsjo added inline comments.Mar 14 2021, 10:14 PM
libcxx/docs/BuildingLibcxx.rst
127–131

Sounds good to me, but I'd like two minor tweaks: "If you are running in an MSYS2 shell and you have installed the MSYS2-provided clang package" - the case of having MSYS2 available (with that package installed) but not using it for testing is maybe contrieved, but I'd prefer clarity.

Secondly, I'd add an "e.g." before the cmake option, as the architecture in the triple merely is a probable suggestion. (I see I didn't have that in earlier versions either, but looking at it now, I'd like to add it.)

mstorsjo updated this revision to Diff 330564.Mar 14 2021, 11:48 PM

Rewrote the paragraph on msys2/clang++/triples as discussed

Quuxplusone accepted this revision.Mar 15 2021, 6:45 AM
Quuxplusone added inline comments.
libcxx/docs/BuildingLibcxx.rst
127–131

the case of having MSYS2 available (with that package installed) but not using it for testing is maybe contrived

No, I agree; if that makes a difference, then it's important to mention!

I'm not thrilled by "you should add e.g. -DLIBCXX_TARGET_TRIPLE=x86_64-windows-msvc to the cmake command line", because again that feels like I'm being asked to do something vaguely unspecified — like, "Try this, but maybe secretly you should have tried something different, so if it doesn't work it's probably your fault not ours." ;) Still, as I don't know what other option might work, I have no concrete suggestion for better wording. I'd say add the "e.g." if it improves the technical correctness, and then :shipit:

mstorsjo added inline comments.Mar 15 2021, 7:02 AM
libcxx/docs/BuildingLibcxx.rst
127–131

The concern in the triple is mainly about the architecture part, I really hate hardcoding assumptions about architectures. (While x86_64 is the primary one readers of the guide would use, i386, arm and aarch64 are in common on windows too.)

mstorsjo added inline comments.Mar 15 2021, 7:18 AM
libcxx/docs/BuildingLibcxx.rst
127–131

I guess one could make it even clearer with "you should add e.g. -DLIBCXX_TARGET_TRIPLE=x86_64-windows-msvc (replacing x86_64 with the architecture you're targeting)", unless that's distracting the reader too far away from the point.

Quuxplusone added inline comments.Mar 15 2021, 7:54 AM
libcxx/docs/BuildingLibcxx.rst
127–131

Sure, that sounds reasonable to me.