-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Use common CLR_CMAKE_* variables in more places #31659
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
Conversation
In some places
I haven't looked at the content of the PR yet... Since it is marked Draft... |
|
Darwin: the official (renewed) OS name is macOS, and base system for iOS, macOS, tvOS etc. is Darwin. CMake,
Please note that PR only deals with the CLR cmake platform variables; the symbols/macros defined for the code have not changed by this PR; although with convergence in place, it is now easy to form a convention / bring about uniformity there as well (e.g. use Failures look unrelated:
|
looks like it is already started: #31668 👍 |
sdmaclea
left a comment
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.
Before this change WIN32 was configured by the HOST compiler and platform. This change is switching it all to be controlled by TARGET platform.
In general the safer rename would be to introduce a CLR_CMAKE_HOST_WIN32 variable. Then use that for the WIN32 rename.
I reviewed the first several dozen files and there were some cases where your rename was correct and other where it was incorrect.
As I mentioned previously. The heuristic I have been using is that compiler setting/configuration should be tied to HOST. This includes things like MT, WIN32, WIN32_LEAN_AND_MEAN. It also includes choice of assembler and assembly language. It would also include references to HOST specific libraries. kernel32.lib ...
I would prefer either
- Rename
WIN32toCLR_CMAKE_HOST_WIN32 - Audit all uses of
WIN32and correctly selectHOSTorTARGET
We can open an issue to toggle CLR_CMAKE_HOST_WIN32 to CLR_CMAKE_TARGET_WIN32 as needed later if you chose the first option.
The review was getting repetitive and I don't think I was adding useful comments. So I stopped reviewing...
|
Thanks for your persistence, @sdmaclea! I went with preference no. 2. :) |
|
@jkotas, should I open an issue? |
|
The crash is #31809 |
|
There were few others crashing found in the history (since 01/26/2020) in Linux as well, so I have filed #31812. |
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.
Looks much better. Thanks
There are a few places where the CLR_CMAKE_TARGET_ is used to select HOST specific libraries. These should be fixed. I stopped commenting on the issue after a while, but I did look through all the files.
sdmaclea
left a comment
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 Thanks.
Fix DAC cross OS compilations regressions introduced by dotnet#31659
Fix DAC cross OS compilations regressions introduced by #31659
eng/native/configureplatform.cmakein libraries (coreclr and installer are already using it).src/libraries/Native/Unix/CMakeLists.txt.CLR_CMAKE_{ARCH_}{HOST,TARGET}_ARMV71andCLR_CMAKE_{HOST,TARGET}_ANDROID.CMAKE_SYSTEM_PROCESSORandCMAKE_SYSTEM_NAMEwith already definedCLR_CMAKE_*counterparts.if(APPLE)andif(WIN32)withCLR_CMAKE_TARGET_DARWINandCLR_CMAKE_TARGET_WIN32respectively.