-
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
[Android] Add callback to host-runtime contract for getting assembly data #112705
Changes from all commits
7b144f3
06c1c83
62189fc
f9f7b90
2a1535e
10e88b0
c2b4eac
aebb13f
766af9b
4f9e680
2509f3f
37d326f
ee65f01
c17e21e
83d394b
0f53984
55cc33c
0b007c3
686bace
6f01f1d
6143339
d662757
73fd37e
6f32ec3
439962d
0e6fe94
d69a550
5895ad8
d4cdf66
75c6e4c
91772f4
32ee9a9
4026a7b
cc4e734
0650ae7
562c8e9
493a186
3e97a9e
f398c6e
7127b59
e1e7e32
58b2698
7d4254a
a16e3ed
99e2bbf
cf72fbe
f286e15
1533d9f
1cdb0ce
5a97aff
885d117
54ad33c
2821e75
ff9021d
38c6376
8c60c1a
57553d4
dfc64db
656a6fd
d890acc
f4fe37a
cf2aae6
2222efe
7c0350b
331e849
2dcd170
1df4b0d
bbf5e47
792f95e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we have changes in hosting header files (this one and @elinor-fung @jkotas what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have been treating the coreclr.dll hosting interface as an internal detail that is not officially documented and that is not guaranteed to be stable. Are you suggesting that we change that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we ship headers and still consider them not guaranteed to be stable? That's what mono does with certain parts of its hosting API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition to what Steve said: Currently, on dotnet/macios side the build downloads the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's always the option of sharing a transport package if we don't want to ship it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want the internal coreclr interface as part of the shipped runtime pack. We do include public hosting headers (coreclr_delegates.h, hostfxr.h, nethost.h) in the host pack. Transport package seems reasonable if we need a better way for other repos to get at internals. |
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 think these changes may break source-build, as it effectively regresses #35727. cc: @omajid can you take a look?
@grendello What was the motivation behind this change?
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 is expanding the coverage of #35727 to cmake configure step. How is it regressing, do you have an example?
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.
From #35727:
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.
@jkoritzinsky
*FLAGS
must be exported before cmake configuration run, otherwise they will be ignored. Where they were previously exported, they were essentially ignored. My motivation, tbh, was to get color in errors and warnings working, but it just seemed to be a mistake to have the exports without any effect. I think it may also be a requirement for packagers in some Linux distributions. For instance, Debian can specify standard system-wide package compilation flags via various dpkg scripts and it's done by pre-setting the*FLAGS
environment variables.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 PR extra flags did had effect but only over
cmake --build
step, not the config step.If something is breaking because of this, it is most likely the problem which needs to be fixed in that 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.
@omajid The policies talk about build, but they don't mean cmake build stage. cmake is a build system generator, so it takes the
*FLAGS
into account only when generating the make/ninja build files, during cmake configuration phase. Here's dotnet/runtime'sbuild.ninja
:As you can see, there's no mention of any of the
*FLAGS
- all of them were expanded and placed by cmake configuration stage in the build file.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.
@omajid if setting some flags at configure time breaks .NET tests, then the problem is in those tests and it should be fixed there. If any component is incompatible with some flags, it can (and should) check for and remove the offending flags/defines. This post gives a potential solution (not tested by me):
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.
@omajid, somehow it stopped working. The cmake generated makefiles (or ninja files) do not explicitly account for $CFLAGS etc. from environment variable anymore (assuming it was working when the original issue was fixed):
with
docker build . -t foo 2>&1 | tee
both ninja and make (penultimate and last lines) show us:
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.
Should we revert the part of this change that impacts source build? Or are we ok with fixing outside of runtime?
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.
Thanks, @am11 and @grendello !
@steveisok said:
Based on the comments and feedback, I think we should fix it as a separate PR. We don't know what that exactly looks like, right now.