-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Minimum CMake downgrade for Linux #39044
Conversation
…here the new one isn't implemented yet.
Tagging subscribers to this area: @ViktorHofer |
Thanks for this PR! I tried building
Please let me know if there's anything I can do to help. |
I've fixed the issues that I've found relatively simple (removing the unknown policy and removing the extra cmake_minimum_required), but I'm not sure how to fix the |
Not off the top of my head. I will dig around. |
The object libraries cannot be used as regular libraries to link to prior to cmake 3.14. All places need to be changed so that the objects from the object libraries are added to the respective targets like source files. E.g.:
|
I am having trouble building with 3.6.2. Seems that https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/eventing/CMakeLists.txt#L11 requires cmake 3.8+. How did you get around this @jkoritzinsky |
I forgot about that. I need to roll that back to use the old find python module. |
@safern can you take a look at the libraries build failures on OSX? |
I've pushed a fix. The problem was that the VM_HEADERS_WKS_ARCH_ASM that contained the asmconstants.h was being added to VM_SOURCES_WKS_ARCH_ASM, which is a set of files that are to be compiled with assembler. It needed to be added to VM_SOURCES_WKS instead (which is done just to let the header appear in the visual studio solution explorer, cmake is clever enough to not to pass .h files to the C++ compiler). |
@omajid can you do a test build against the RHEL 7 image you'll be building against for the distro build? |
Of course. I have tested on RHEL 8 using this PR (it works!). I am currently testing on RHEL 8 end-to-end now (using source-build Preview 4 + a manual backport of this patch). If/when that works, RHEL 7 is next on my list. |
A diff --git a/src/installer/corehost/CMakeLists.txt b/src/installer/corehost/CMakeLists.txt
index 178d8ab..46d1c5e 100644
--- a/src/installer/corehost/CMakeLists.txt
+++ b/src/installer/corehost/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.6.3)
+cmake_minimum_required(VERSION 3.6.2)
project(corehost)
diff --git a/src/libraries/Native/Unix/CMakeLists.txt b/src/libraries/Native/Unix/CMakeLists.txt
index dee9e51..3168c7e 100644
--- a/src/libraries/Native/Unix/CMakeLists.txt
+++ b/src/libraries/Native/Unix/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.6.3)
+cmake_minimum_required(VERSION 3.6.2)
if(CLR_CMAKE_TARGET_IOS OR CLR_CMAKE_TARGET_TVOS)
# CMake 3.14.5 contains bug fixes for iOS
cmake_minimum_required(VERSION 3.14.5)
diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt
index c75e5db..77fcc16 100644
--- a/src/tests/CMakeLists.txt
+++ b/src/tests/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.6.3)
+cmake_minimum_required(VERSION 3.6.2)
cmake_policy(SET CMP0042 NEW)
project(Tests) Edit: I am still working on backporting this to preview 4 so I can test end-to-end with source-build (on RHEL 7 and RHEL 8). |
I've applied your patch. Marking this PR as ready for review/non-work in progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The non-coreclr specific changes look good to me (can't vouch for the coreclr changes).
OBJECT | ||
${BCLTYPE_SOURCES} | ||
) | ||
|
||
add_dependencies(bcltype eventing_headers) | ||
add_dependencies(bcltype_obj eventing_headers) | ||
add_library(bcltype INTERFACE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An idea for a future cleanup: This add_library / target_sources pair pattern is used at 24 places. It may be nice to clean this up in a follow up change by creating a cmake function for this pattern. Or even encapsulate the add_library_clr in that function, getting something along these lines:
add_intermediate_library_clr(bcltype ${BCLTYPE_SOURCES})
add_intermediate_library_dependencies(bcltype eventing_headers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should clean this up in the future by making some additional functions.
@@ -30,6 +30,8 @@ set(HEADERS | |||
../../../hostfxr_resolver.h | |||
) | |||
|
|||
list(APPEND SOURCES $<TARGET_OBJECTS:libhostfxr_static> $<TARGET_OBJECTS:libhostpolicy_static>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why was this change needed. Linking to the static libraries should work fine. The only case when we need to use object libraries is when a static library is built as a combination of other libraries, but it is not the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The libhostfxr_static
and libhostpolicy_static
targets are object libraries. The corehost CMake infra could really use some updates and cleanup to be more straightforward. I'd like to do that in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, interesting. I can see that they were changed to object ones few months ago and I am not sure why.
I agree that the corehost cmakefiles are very difficult to mentally parse. They looks as if msbuild style of build programming was used here. Instead of using functions for doing common stuff, ambient cmake variables are being set and then common helper cmake files included. I'd love to see it transformed to a natural cmake way at some point.
src/libraries/Native/Unix/System.Globalization.Native/CMakeLists.txt
Outdated
Show resolved
Hide resolved
cc @mmitche |
@janvorli any more comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
@jkoritzinsky no, I have thought I've already approved it, but I can see I haven't. So I've just done that. |
Thank you @jkoritzinsky for driving this effort! |
@omajid which version of clang are we using on redhat 7 for source build? We noticed that by default centos 7 installs 3.4 which we do not support building with. |
We have generally used This is how we are building 3.1 with |
Clean builds using the following:
|
@jeffschwMSFT Was this supposed to be ported to p8? |
I did a port to preview 4, so I could build against older cmake versions using source-build. Source-build is currently at 5.0 Preview 4. Port is available here: https://github.com/omajid/runtime/tree/5.0-preview4-cmake-backport |
Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com> Co-authored-by: Jan Vorlicek <janvorli@microsoft.com>
Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com> Co-authored-by: Jan Vorlicek <janvorli@microsoft.com> Co-authored-by: Jeremy Koritzinsky <jekoritz@microsoft.com> Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com> Co-authored-by: Jan Vorlicek <janvorli@microsoft.com>
Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com> Co-authored-by: Jan Vorlicek <janvorli@microsoft.com>
Downgrade the minimum version of CMake to 3.6.2 for our Linux builds and change our CMake scripts appropriately:
Currently completed steps:
add_compile_definitions
since it is easily polyfillableadd_link_options
since it is not easy to create our own implementation of that functions the same. Move back to setting linker options via the flag properties.So far I've only undone the changes that I made in dotnet/coreclr#26980 that I know are required. @janvorli can you help out with some of the other issues that we'll encounter when lowering the CMake version? I don't know all of the corner case behaviors that we've started to depend on in the last 10ish months.
Validation that we actually build on CMake 3.6.2 will come when we get a new Docker image has CMake 3.6.2 (and no newer) installed (@jashook is working on this).
Contributes to #38755
Depends on dotnet/arcade#5770
cc: @dleeapho @janvorli @jashook @omajid @jkotas @tmds @wfurt @RheaAyase