-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Building singlefilehost with clr partition. #48254
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
325bf4d
to
488ee21
Compare
9839004
to
04bf9bb
Compare
I think this is ready for review. |
@@ -348,14 +348,14 @@ else() | |||
clr_unknown_arch() | |||
endif() | |||
|
|||
set(SOURCES | |||
set(JIT_CORE_SOURCES |
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.
This change was needed to not include resources in the statically linked JIT.
@@ -42,6 +42,20 @@ include(../../fxr/files.cmake) | |||
include(../../hostpolicy/files.cmake) | |||
include(../../hostcommon/files.cmake) | |||
|
|||
if(MSVC) | |||
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/wd4996>) |
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.
these /wd
defined for everything in host.native .
Since we build this separately from others now, I had to add the options here.
get_property(ALL_COMPILE_OPTIONS TARGET ${PROJECT_NAME} PROPERTY COMPILE_OPTIONS) | ||
string(TOUPPER ${RUNTIME_CONFIG} UPPERCASE_RUNTIME_CONFIG) | ||
|
||
# make sure that certain compiler and linker settings match the runtime config |
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 am very happy that this is gone. It seemed very fragile.
6385035
to
a874f3a
Compare
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!
Thanks!!! |
Hello @VSadov! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
@VSadov It appears this change broke the JIT rolling build (which only builds the JIT, using, for example, https://dev.azure.com/dnceng/internal/_build/results?buildId=1004132&view=results cc @dotnet/jit-contrib |
@BruceForstall, I have opened #48552 with the fix. The condition just need to include corehost directory conditionally. |
NOTE: static libs for the native libraries that are built in clr partitions are still installed into artifacts.
I left them be, in case there are other uses, but singlefilehost no longer needs them.
Resolves:#43700