Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrappers: add CMake toolchain files #430

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nolange
Copy link
Contributor

@nolange nolange commented Jun 3, 2024

Those toolchain files greatly simplify configuring a project to use the llvm-mingw toolchain.

The toolchain file will:

  • Configure CMake to look for libraries and headers in the correct directories
  • Configure Clang for the architecture, libc++ and lld
  • Locate the primary Compiler executable

CMake has some quite complete and complex machinery to detect further tools
and configuration. Whatever supported in your CMake version should "just work" (eg. C++ Modules one day).

The lookup uses the system clang toolchain as fallback, and does not require any of the wrappers / symlinks in bin.
ie. Its possible to delete the bin directory if a matching clang/llvm toolchain is installed.

@nolange nolange force-pushed the add_cmake_toolchain_files branch from 07c85c6 to 1e9c908 Compare June 3, 2024 17:24
@nolange nolange marked this pull request as draft June 6, 2024 19:38
@nolange nolange marked this pull request as draft June 6, 2024 19:38
@nolange nolange marked this pull request as draft June 6, 2024 19:38
@nolange
Copy link
Contributor Author

nolange commented Jun 6, 2024

still needs some work

@nolange nolange force-pushed the add_cmake_toolchain_files branch from 1e9c908 to 5030a66 Compare June 7, 2024 20:51
@nolange nolange marked this pull request as ready for review June 7, 2024 20:51
@nolange
Copy link
Contributor Author

nolange commented Jun 7, 2024

Had to fix some issues, mostly on windows (needs to convert to native paths) and some quoting.

Something of note is, that CMake does locate matching tools by searching first for the version-qualified name. For example it will look for llvm-ranlib-18 then llvm-ranlib. Means that is available, a system llvm-ranlib-18 will be used.
This is not optimal but is unlikely to cause big trouble.

Still, ideally llvm-mingw could provide llvm-ranlib-18, making these lookup schemes more robust.

@nolange nolange marked this pull request as draft June 12, 2024 09:08
@nolange nolange marked this pull request as ready for review June 18, 2024 07:35
@nolange
Copy link
Contributor Author

nolange commented Jun 18, 2024

i dropped the support of using a system clang/llvm installation. its pretty hard to get right on windows, i am not even sure if its bc if bugs or if some antivirus/company it policies intervene in my testing.

@nolange
Copy link
Contributor Author

nolange commented Jun 23, 2024

I added some more stuff in another branch, primary to add some unit tests:
https://github.com/nolange/llvm-mingw/commits/new_toolchainfile/

There is a subdirectory test/cmake/tests building the existing tests. I am probably biased but this should be way cleaner than the Makefile and offers integration with ctest and all the benefits that come with CMake (like just opening the directory in vscode and have everything setup automatically).

And another test/cmake/template which should serve as a basic project that works with llvm, msvc and a series of IDEs (Visual Studio).

@nolange
Copy link
Contributor Author

nolange commented Jul 31, 2024

Ping?

@mstorsjo
Copy link
Owner

So, I'm quite undecided about this (as I think I had mentioned in some other discussion, where you mentioned that you were going to submit this).

The primary way of using the llvm-mingw tools is via the triple prefixed wrappers - this is easy to set up and use with all build systems, and matches how many other cross toolchains work. We probably don't want to ship config files for all build systems out there.

Merging this, even if I'm not a fan of using toolchain files, would cost us a bit more files to maintain and test. (I did see that you had ported all my smoke tests to run on cmake - not sure if I'd go with that or just something smaller. In any case, I'm pretty sure I want to keep the original plain makefile based test setup anyway.) For people using CMake, it would indeed make things simpler to use though.

Regarding getting the right defaults for -stdlib=libc++ and such, for other tools than the plain compiler - that is also possible by using clang config files; see https://github.com/mstorsjo/llvm-mingw/commits/clang-cfg - I'm pondering merging something like that. (Unfortunately, that doesn't work for the UWP defaults, as clang normalizes -mingw32uwp triples into the same windows-gnu as regular triples, so the UWP additions still need to be supplied somewhere.)

So, I'm undecided for now.

@nolange
Copy link
Contributor Author

nolange commented Aug 1, 2024

The primary way of using the llvm-mingw tools is via the triple prefixed wrappers - this is easy to set up and use with all build systems, and matches how many other cross toolchains work. We probably don't want to ship config files for all build systems out there.

I don't agree with "easy to setup with all build systems", but I don't want to remove the wrappers. CMake is wildy popular (so is QT), so that's a reason to add support.
Using CMake + toolchain file it further is also easy to setup with IDEs, package-managers like Conan, static analyzers like CodeChecker, etc. Most of the time you don't need to configure anything for a correctly setup CMake Project.

Merging this, even if I'm not a fan of using toolchain files, would cost us a bit more files to maintain and test.

It saves many more people work if the CMake support is central, and you save documenting/answering on how to use the toolchain with CMake. I could add a sample project for illustration.

Regarding getting the right defaults for -stdlib=libc++ and such, for other tools than the plain compiler - that is also possible by using clang config files.

Yes, I played with that but its completely irrelevant here (could replace the ugly Windows wrappers potentially). I see you have some problems with a separate uwp configuration, AFAIR you could use clang -v -target x86_64-w64_uwp-windows-gnu then it will search for x86_64-w64_uwp-windows-gnu.cfg. I got the branch stowed somewhere, need to dig it up.

Also these default-things typically are quite fragile and inflexible regarding typical workflows. For ex. building QT dependencies in a STAGING prefix, then QT itself and installing it to the toolchain (or creating an additional sysroot) are all quite nicely abstracted by setting a few CMake variables. Using raw commandline is alot more complicated and the wrappers make this worse not better.

@mstorsjo
Copy link
Owner

mstorsjo commented Aug 1, 2024

Regarding getting the right defaults for -stdlib=libc++ and such, for other tools than the plain compiler - that is also possible by using clang config files.

Yes, I played with that but its completely irrelevant here

It's relevant to the point whether using toolchain files solves a technical issue (getting the right defaults flags for -stdlib=libc++ to clang-tidy or similar) or just a convenience issue.

I see you have some problems with a separate uwp configuration, AFAIR you could use clang -v -target x86_64-w64_uwp-windows-gnu then it will search for x86_64-w64_uwp-windows-gnu.cfg.

Right, that could work, but feels a bit ugly and would also deviate even more from earlier conventions. And having a differing vendor field may trip up a number of other cases (we may potentially use the "per-target runtime" install layout for compiler-rt and/or libcxx at some point, and at that point, changing the vendor field would make it fail to find all those runtimes). But thanks for the idea!

@nolange
Copy link
Contributor Author

nolange commented Aug 1, 2024

It's relevant to the point whether using toolchain files solves a technical issue (getting the right defaults flags for -stdlib=libc++ to clang-tidy or similar) or just a convenience issue.

toolchain files solves a technical issue, namely configuring CMake so that Projects can do the right decisions, including building dependencies with the toolchain file (or passing it to a package manager for the same reason).
The toolchain file is then a single parameter defining the target system.

Personally I switched to CMake at work many years ago, having projects dealing with Barebones RTOS on microcontrollers, Emdedded Linux to currently also the Microsoft abominations. Now you don't have to care for the target or project, you can open it in one of the many IDEs supporting CMake and everything works including all the tools expecting to find the used flags in the commandline (compile_commands.json). Cause you cant expect the used compiler/wrapper itself being relevant - tools often want to know what the meaning of the command is without invoking the compiler/wrapper then do something entirely different with this information.

Of course you could argue it could be done differently, but that's almost equivalent to saying don't use CMake and all the integration it provides.

endif()

if(NOT CMAKE_SYSTEM_NAME)
set(CMAKE_SYSTEM_NAME Windows)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to CMake documentation, setting CMAKE_SYSTEM_NAME manually has the side effect of kicking CMake into a "cross-compilation mode", i.e. CMAKE_CROSSCOMPILING will be set to true. IMO it shouldn't be set when compiling natively, because some projects have cross-compilation logic that involve building host tools separately and you will end up building things twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats pretty vague, cross-compiling is not just a different System.

  • If I run x64 Windows and compile for Arm Windows I do a cross-compilation.
  • What if you dont have UCRT installed, but the toolchain uses UCRT.
  • On Linux you have tons of more differences - different flavors of Linux and runtimes.

I get that you potentially could safe some time, but its not trivial to guarantee the stuff will run on the host and the user will then have a broken build with potentially no useful error message on Windows (like the needed DLLs are not in PATH).

Way too many pitfalls and downsides for me.

The most reliably way to build for host would be to run CMake without params and hope the User prepared for that. (Or use a package manager like Conan which will also cache the tools once they are built).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be important to not set cmake into cross compiling mode when not necessary. I agree that it's not trivial though. One problem is that CMAKE_HOST_SYSTEM_PROCESSOR mostly indicates what architecture CMake itself was compiled as.

I believe the logic could be something like this (in pseudocode):

if (CMAKE_HOST_SYSTEM_NAME == "Windows")
    if (CMAKE_HOST_SYSTEM_PROCESSOR == ${arch})
        # Exact architecture match, not cross compiling
    elif (CMAKE_HOST_SYSTEM_PROCESSOR == x86_64 AND ${arch} == i686)
        # Differing architectures, but should be executable
    elif (CMAKE_HOST_SYSTEM_PROCESSOR == aarch64 AND ${arch} == armv7)
        # Differing architectures, but should be executable. (Although future aarch64 processors may remove 32 bit support.)
    else
        # Cannot execute the built executables, indicate that we're cross compiling.
        set(CMAKE_SYSTEM_NAME "Windows")
    endif
else
    set(CMAKE_SYSTEM_NAME "Windows")
endif()

Yes, this is not water tight for some potential odd combinations of CRT and such, but I think this would be a better user experience (especially wrt compiling something like LLVM itself) rather than always flagging it as cross compilation.

Copy link
Contributor Author

@nolange nolange Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be important to not set cmake into cross compiling mode when not necessary.

I think you overestimate the amount of Projects depending on CMAKE_CROSSCOMPILING, the meaning/usage of the variable seems not fully clear even among CMake Maintainers.
I believe this is legacy Stuff from decades ago.

Its recommended to write tests for compiler features, and you can do the same for stuff you plan to run:

include(CheckCSourceRuns)
check_c_source_runs("int main() {return 0;}" HEY_I_CAN_RUN_THE_STUFF_I_BUILD)

This gives you precisely the info you need (but this only works after the project command and after the toolchain is consumed).

So that's a Project problem first and foremost and the cost is some inefficiency, the adverse effects of CMAKE_CROSSCOMPILING missing when it should be set is a broken build (potentially after hours).
Even then I think it would make more sense to set CMAKE_CROSSCOMPILING and try to use CMAKE_CROSSCOMPILING_EMULATOR to setup PATH.

I believe the logic could be something like this (in pseudocode):

I'm strictly against adding such guesswork. if you really want then I add a separate toolchain for "native" builds.

I had planned to add logic to optionally use a system installed clang in-place, but I'd want simple reliable toolchains as base.
ie. you could extend the toolchain with your logic:

include("${CMAKE_CURRENT_LIST_DIR}/llvm-mingw_toolchainfile.cmake")
if-but-else-if-I-think-it-could-run-native()
  unset(CMAKE_SYSTEM_NAME)
endif()

Copy link
Owner

@mstorsjo mstorsjo Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you overestimate the amount of Projects depending on CMAKE_CROSSCOMPILING, the meaning/usage of the variable seems not fully clear even among CMake Maintainers.
I believe this is legacy Stuff from decades ago.

I disagree with your belief. Yes it's not a huge amount of projects using it, but as someone who has spent a significant amount of time in the CMake files of LLVM and its runtimes, I believe getting this right is kinda vital.

Its recommended to write tests for compiler features, and you can do the same for stuff you plan to run:

include(CheckCSourceRuns)
check_c_source_runs("int main() {return 0;}" HEY_I_CAN_RUN_THE_STUFF_I_BUILD)

This gives you precisely the info you need (but this only works after the project command and after the toolchain is consumed).

Feel free to use such constructs in your CMake projects, but in the ones I've touched, CMAKE_CROSSCOMPILING is the key.

I'm strictly against adding such guesswork. if you really want then I add a separate toolchain for "native" builds.

No, a separate toolchain file sounds much worse. You may dislike the guesswork, but that is the form that it'll be for it to be merged here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I would expect that to be what to be used, when using the “default native” toolchain file, to be using the toolchain default arch, not dependent on what system happens to be reporting.

The issue here is that the toolchain is used in context of CMake, and to me whether clang is 32 or 64 bit is an implementation detail.
You now have 5 toolchain-files, 4 of them will give you the same result no matter which version you installed.

The other should have the same understanding as CMake - and commands file find_package, find_library for the HOST will be aligned to CMAKE_HOST_SYSTEM_PROCESSOR.
And by that targeting that, will also give you the same result no matter which clang version you installed.

You’re missing the point here. If you’ve downloaded a 32 bit toolchain and run it on a 64 bit arm system, the program isn’t that you can’t execute the binaries you’ve just built. You won’t be able to run the compiler in the first place!

Its about defining what a "native" build means. I would argue that targeting a 32bit architecture from 64bit cross-compiling and no fit for the toolchain-file which we claimed to be "native".

The toolchain default arch will always be a reasonable thing use, otherwise you’ve got a fatally incompatible toolchain build.

Unless you use CMake functions which will resolve to 64bit libraries for example.

if we would adopt your change I would rather patch the static cpu architecture into the toolchain-file during build.

Sorry, but that sounds unnecessarily inflexible. The contents of the supposedly arch independent files under share would suddenly have such details hardcoded.

Not sure we talk about the same thing here, I am talking about patching in the final value during running install-wrappers.sh. Not sure you would then later swap clang without the toolchain-files?

Just no. Why go to all that length to check things, just to warn, rather than just pick up the architecture and use that, and silently do the right thing?

I dont get your motivition, you seem to not be inclined to use the toolchain-files, but try to cook up some rather weird setups where they fail and the result should be to add further complications.
I can cook up examples where your "right thing" will cause troubles, especially if the use the right thing within CMake to lookup DLLs.

The real question is: "Why would a user install a 32bit toolchain on a 64bit OS?"
Are there valid reason for this, because if not then this is just a unnecessary problematic mess tying to the definition of "cross-compiling".

I would add a check to test if thats the case, add a warning that "Your toolchain is not matching CMake's host architecture".
With the fineprint that the user should either be able to fix the almost certain troubles, or install the fitting toolchain.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other should have the same understanding as CMake - and commands file find_package, find_library for the HOST will be aligned to CMAKE_HOST_SYSTEM_PROCESSOR. And by that targeting that, will also give you the same result no matter which clang version you installed.

No, find_package and find_library will be aligned with the settings for CMAKE_FIND_ROOT_.... For the strict cross compilation cases, this really shouldn't be looking into anything related to the host system. So why would it in this case, when the only difference is that we're not setting CMAKE_SYSTEM_NAME?

Sorry, but that sounds unnecessarily inflexible. The contents of the supposedly arch independent files under share would suddenly have such details hardcoded.

Not sure we talk about the same thing here, I am talking about patching in the final value during running install-wrappers.sh. Not sure you would then later swap clang without the toolchain-files?

When cross-building the toolchains, we have a couple of cases where we transplant pieces from one toolchain build into another. E.g. when we build the toolchains that will run on Windows, they are cross-compiled on Linux. We don't rebuild the mingw-w64 files with this new cross compiled toolchain (as it can't be executed on that host), but we copy over those compiled files the preexisting Linux based toolchain. So we take a bunch of directories from an existing toolchain and transplant them into a new one.

In that case, we'd still run install-wrappers.sh for the new toolchain and not copy over the output of that, so having the default arch hardcoded in the files wouldn't break any of the current build processes directly. But my point is that it is surprising and unnecessary to have the contents of share be build dependent in that way.

I dont get your motivition, you seem to not be inclined to use the toolchain-files, but try to cook up some rather weird setups where they fail and the result should be to add further complications.

Sorry, but this sounds quite disrespectful.

If I am to merge and maintain toolchain files, I want them to behave in a way that I find reasonable and consistent. The definition of what that means, is up to me.

I don't want to add further complications needlessly - but if I feel that it would be important for what I consider reasonable behaviour, I wouldn't shy away from a little extra complication in one file.

In this case, what I'm suggesting wouldn't add any complication at all, it would remove the whole mapping of CMAKE_HOST_SYSTEM_PROCESSOR to our architecture names, replaced with taking the output of clang -dumpmachine and extracting the arch name from there. That's a simplification in my opinion.

You said yourself that you consider running a 32 bit toolchain on a 64 bit host a contrieved case that you don't care about. Then why do you even bother? Why do you want to add further complications to add a warning, when you can just make it use the arch name from that output and not add a warning?

Copy link
Contributor Author

@nolange nolange Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, find_package and find_library will be aligned with the settings for CMAKE_FIND_ROOT_.... For the strict cross compilation cases, this really shouldn't be looking into anything related to the host system. So why would it in this case, when the only difference is that we're not setting CMAKE_SYSTEM_NAME?

The issue is not cross-compiling, but this one being the "native" toolchain-file.
The project might do a variety of things here (including spawning a process and calling CMake without any flags since well... its "native"), things are already tricky and I doubt changing the architecture helps.
TBH I dont develop an Windows, I dont know how lookup is working there but there are for ex. flags like REGISTRY_VIEW HOST which seem like an obvious violation.

But my point is that it is surprising and unnecessary to have the contents of share be build dependent in that way.

I half agree here, I dont want the contents in share to be build dependent. But I would go further to the function not being build dependent, which it is if you fetch variables from clang.
The value is fixed for a finalized toolchain and in that case, I would prefer doing it once and explicitly having the result readable in plain text.

Sorry, but this sounds quite disrespectful.

My apologies, I thought the remainder should clear that up. You bring up hypothetical problems and questions wheres I stated more than once that I would wait for explicit problems and start with a simple, least surprises toolchain.

Having a "native" toolchain-file that uses at best an ambiguous definition of that (Id call targeting 32bit on 64bit cross-compiling), while correctly targeted cross-compile toolchain-file's are available doesnt make sense to me.

If I am to merge and maintain toolchain files, I want them to behave in a way that I find reasonable and consistent. The definition of what that means, is up to me.

I try to explain why that's the opposite, given that I explicitly use CMake and Toolchains for a while, including llvm-mingw and the proposed toolchains.

In this case, what I'm suggesting wouldn't add any complication at all, it would remove the whole mapping of CMAKE_HOST_SYSTEM_PROCESSOR to our architecture names, replaced with taking the output of clang -dumpmachine and extracting the arch name from there. That's a simplification in my opinion.

It's another dynamic behavior on top of CMake's logic, potentially breaking assumptions.

You said yourself that you consider running a 32 bit toolchain on a 64 bit host a contrieved case that you don't care about. Then why do you even bother? Why do you want to add further complications to add a warning, when you can just make it use the arch name from that output and not add a warning?

You brought the case up, and I'd rather want to infer feedback from users when they run into problems and what they want to try to do.
Cause I think that's the best thing if you want to maintain the toolchain. I doubt its a problem, I doubt your solution will help and I am not keen on adding stuff I haven't seen done anywhere just on a hunch.

Its also easier to word:

  1. Target your "native" System (including bitness), use llvm-mingw_toolchainfile.cmake
  2. Else take the fitting cross toolchain

You heave the last word, but this is my recommendation for maintaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fetching dumpmachine would look like this:

if(NOT CMAKE_SYSTEM_PROCESSOR)
  set(CMAKE_SYSTEM_PROCESSOR ${CMAKE_HOST_SYSTEM_PROCESSOR})
  if(CMAKE_HOST_WIN32)
    execute_process(COMMAND "${_prefix}bin/clang.exe" -dumpmachine
      RESULT_VARIABLE cresult
      OUTPUT_VARIABLE _respath
      OUTPUT_STRIP_TRAILING_WHITESPACE
    )
    if(NOT cresult EQUAL 0)
      message(FATAL_ERROR "\"${_prefix}bin/clang.exe\" -dumpmachine: failed with ${cresult}")
    endif()
    unset(cresult)
    string(REGEX REPLACE "-.*" "" CMAKE_SYSTEM_PROCESSOR "${_respath}")

Do you know what clang -dumpmachine returns on arm? Is it armv7 or something different?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that looks reasonable.

Yes, it returns armv7 for those targets, and i686 in for such toolchains (it's not normalized into i386) - so we should be able to reuse the literal spelling of the arch here without any remapping. (In this case, it's also possible to test this by running e.g. armv7-w64-mingw32-clang -dumpmachine and see what that outputs.)

@nolange
Copy link
Contributor Author

nolange commented Aug 9, 2024

I updated the branch with the CMake Testsuite: https://github.com/nolange/llvm-mingw/commits/new_toolchainfile/

Now should work (almost) out of the Box with Editors supporting CMake Projects. Likely will need to setup the path to the toolchain.

The Project itself should compile with llvm-mingw, gcc-mingw and even with (wine-)msvc. Running tests is supported if wine is available, but some might have trouble finding DLLs without help.

Hope this illustrates why providing toolchain-files allows a ton of integrations that are difficult to impossible without.

@mstorsjo
Copy link
Owner

mstorsjo commented Aug 9, 2024

I think I can agree that these toolchain files could be useful to some people, even if it is not how I use cmake. So perhaps we can aim towards trying to get this merged sometime during the LLVM 19.x releases.

As for the testsuite, I appreciate your effort in trying to wrap that up in cmake. I don't think I would like to merge those changes though.

I'll try to have a look around and see what kind of testing I'd like to have, within the scope of this project - if I go with a really bare-bones cmake test file like https://github.com/mstorsjo/msvc-wine/blob/master/test/CMakeLists.txt or something else, and how I would like it to be hooked up in the CI environment.

So sure, we probably can do this, but give me some time to integrate it in a way I'm comfortable with.

@nolange
Copy link
Contributor Author

nolange commented Aug 9, 2024

I think I can agree that these toolchain files could be useful to some people, even if it is not how I use cmake. So perhaps we can aim towards trying to get this merged sometime during the LLVM 19.x releases.

That is my hope

As for the testsuite, I appreciate your effort in trying to wrap that up in cmake. I don't think I would like to merge those changes though.

I haven't done a merge request?
Its both for illustrating a point, namely if you use CMake more thoroughly then a toolchain gets pretty important. But then things get easier using a different compiler (mstorsjo/msvc-wine#132).
The other point was to ease your concern of maintenance (testing) of the toolchain files... but I guess now you have more concerns about maintaining the CMakeFiles.txt ;)

@mstorsjo
Copy link
Owner

mstorsjo commented Aug 9, 2024

As for the testsuite, I appreciate your effort in trying to wrap that up in cmake. I don't think I would like to merge those changes though.

I haven't done a merge request? Its both for illustrating a point, namely if you use CMake more thoroughly then a toolchain gets pretty important. But then things get easier using a different compiler (mstorsjo/msvc-wine#132). The other point was to ease your concern of maintenance (testing) of the toolchain files... but I guess now you have more concerns about maintaining the CMakeFiles.txt ;)

Yes, you haven't done a PR for those bits, but I was just proactively mentioning that I don't think I'd want to maintain that bit, even if it can serve as a useful example.

@nolange
Copy link
Contributor Author

nolange commented Aug 19, 2024

Did alot more tests, was able to build and later find openssl3 + qt6 via pkgconf and cmake. Needed some fixes.

Those toolchain files greatly simplify configuring a project
to use the llvm-mingw toolchain.
@nolange
Copy link
Contributor Author

nolange commented Aug 20, 2024

Hmm, come to think of it - I would really like if those files could be indiscriminately used and abused as templates, without any licensing concerns. Your project has a lengthy Apache2 license, I marked them as Public Domain. Hope that's fine with you

@mstorsjo
Copy link
Owner

Hmm, come to think of it - I would really like if those files could be indiscriminately used and abused as templates, without any licensing concerns. Your project has a lengthy Apache2 license, I marked them as Public Domain. Hope that's fine with you

It's not apache2, it's the ISC license (which is equivalent with simplified BSD and MIT) for the build scripts etc. It's meant as the simplest and least restrictive license that still is a proper license.

I've heard that some people have concerns around "public domain" (which isn't really a thing in all jurisdictions), but I guess it should be fine for these files, especially if users are meant to copy and adapt them. (And if someone has a problem with it, we can add the same ISC license header as the other files.)

@nolange
Copy link
Contributor Author

nolange commented Aug 20, 2024

It's not apache2, it's the ISC license (which is equivalent with simplified BSD and MIT) for the build scripts etc. It's meant as the simplest and least restrictive license that still is a proper license.

My bad I looked at the release Archive which ships apparently with LLVM's LICENSE.TXT (?).
I would definitely add a Header since otherwise its not apparent, to me those files are just a bit more complex but still "obvious" config files. Still would prefer no license (less hassle with updating authors), but I am not terrible against MIT/BSD.

@mstorsjo
Copy link
Owner

It's not apache2, it's the ISC license (which is equivalent with simplified BSD and MIT) for the build scripts etc. It's meant as the simplest and least restrictive license that still is a proper license.

My bad I looked at the release Archive which ships apparently with LLVM's LICENSE.TXT (?).

Yep - the build scripts themselves don't ship in the distributable release packages, so the license of llvm-mingw isn't really visible there, it's mainly the LLVM and mingw-w64 licenses.

I would definitely add a Header since otherwise its not apparent, to me those files are just a bit more complex but still "obvious" config files. Still would prefer no license (less hassle with updating authors), but I am not terrible against MIT/BSD.

Ok, let's go with this public domain form for now, and we can always revisit it if someone else is against it.

@mstorsjo
Copy link
Owner

So, I have integrated some smoke testing of these files, and tested it on github actions. See https://github.com/mstorsjo/llvm-mingw/commits/cmake-toolchain-files for what I could consider to merge.

There's two changes on top of your toolchain files; we don't want to set CMAKE_SYSROOT, because on the Windows toolchain, we have includes in <prefix>/include, and if we pass --sysroot pointing at the <arch>-w64-mingw32 subdir, it won't find the headers.

Secondly, cmake does weird things, reincluding the toolchain file many times. After concluding that we're not cross compiling, it gets reincluded, with CMAKE_SYSTEM_PROCESSOR reset to the default CMAKE_HOST_SYSTEM_PROCESSOR, so we end up with it set to AMD64 - so I had to extend the condition for when we fetch our default value for this.

With these changes in place, the smoke tests run as desired, both in full cross compilation mode, and with the toolchain default native arch.

I could go ahead and merge these changes soon (after the LLVM 19.1.0 RC 3 package gets completed, which is running in CI right now) if you don't have anything extra in mind right now.

@nolange
Copy link
Contributor Author

nolange commented Aug 20, 2024

So, I have integrated some smoke testing of these files, and tested it on github actions. See https://github.com/mstorsjo/llvm-mingw/commits/cmake-toolchain-files for what I could consider to merge.

There's two changes on top of your toolchain files; we don't want to set CMAKE_SYSROOT, because on the Windows toolchain, we have includes in <prefix>/include, and if we pass --sysroot pointing at the <arch>-w64-mingw32 subdir, it won't find the headers.

CMAKE_SYSROOT is essential because that's what CMake and other tools will use - this ithe point of this PR.
I will look at how I best add this path to the FLAGS, but it will take a while to retest everything =/.

Secondly, cmake does weird things, reincluding the toolchain file many times. After concluding that we're not cross compiling, it gets reincluded, with CMAKE_SYSTEM_PROCESSOR reset to the default CMAKE_HOST_SYSTEM_PROCESSOR, so we end up with it set to AMD64 - so I had to extend the condition for when we fetch our default value for this.

Yes it does funny things, including spawning separate CMake-Processes during try_compile using only a part of the information and I bet the version always using CMAKE_HOST_SYSTEM_PROCESSOR will cause less trouble while making only a difference in corner cases no one should care about.

With these changes in place, the smoke tests run as desired, both in full cross compilation mode, and with the toolchain default native arch.

I could go ahead and merge these changes soon (after the LLVM 19.1.0 RC 3 package gets completed, which is running in CI right now) if you don't have anything extra in mind right now.

Well, this tc won't work for me, and I am gonna find some time finding a fix and retesting stuff.

@mstorsjo
Copy link
Owner

Secondly, cmake does weird things, reincluding the toolchain file many times. After concluding that we're not cross compiling, it gets reincluded, with CMAKE_SYSTEM_PROCESSOR reset to the default CMAKE_HOST_SYSTEM_PROCESSOR, so we end up with it set to AMD64 - so I had to extend the condition for when we fetch our default value for this.

Yes it does funny things, including spawning separate CMake-Processes during try_compile using only a part of the information and I bet the version always using CMAKE_HOST_SYSTEM_PROCESSOR will cause less trouble while making only a difference in corner cases no one should care about.

Sorry, but re-repeating that this is a corner case that nobody should care about will render this PR closed. Get over it.

@mstorsjo
Copy link
Owner

So, I have integrated some smoke testing of these files, and tested it on github actions. See https://github.com/mstorsjo/llvm-mingw/commits/cmake-toolchain-files for what I could consider to merge.
There's two changes on top of your toolchain files; we don't want to set CMAKE_SYSROOT, because on the Windows toolchain, we have includes in <prefix>/include, and if we pass --sysroot pointing at the <arch>-w64-mingw32 subdir, it won't find the headers.

CMAKE_SYSROOT is essential because that's what CMake and other tools will use - this ithe point of this PR. I will look at how I best add this path to the FLAGS, but it will take a while to retest everything =/.

FWIW, for quicker iteration with testing, I did testing with a separate patch on top, that skips actually building the toolchain and just adds the files on top of an existing toolchain release: cmake-toolchain-files-testing

@nolange
Copy link
Contributor Author

nolange commented Aug 20, 2024

So, I have integrated some smoke testing of these files, and tested it on github actions. See https://github.com/mstorsjo/llvm-mingw/commits/cmake-toolchain-files for what I could consider to merge.
There's two changes on top of your toolchain files; we don't want to set CMAKE_SYSROOT, because on the Windows toolchain, we have includes in <prefix>/include, and if we pass --sysroot pointing at the <arch>-w64-mingw32 subdir, it won't find the headers.

CMAKE_SYSROOT is essential because that's what CMake and other tools will use - this ithe point of this PR. I will look at how I best add this path to the FLAGS, but it will take a while to retest everything =/.

FWIW, for quicker iteration with testing, I did testing with a separate patch on top, that skips actually building the toolchain and just adds the files on top of an existing toolchain release: cmake-toolchain-files-testing

No, the problem is building several CMake projects, see if they resolve the correct paths vie CMake and pkg-conf and if IDE and tools like clangd (not the binary from llvm-mingw) that solely depend on compile_commands.json fully work.

that includes the stuff like multiple implicit invocations of CMake and multiple passes over the toolchain file. I am not confident until I used it for a while.

@nolange
Copy link
Contributor Author

nolange commented Aug 20, 2024

Adopted TC passes the compile tests: https://github.com/nolange/llvm-mingw/tree/test_tc

Still need to see if there's fallout when using more... CMake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants