[build] Move Mono API headers under src/native/public #64569

lambdageek · 2022-01-31T21:38:35Z

Contributes to #64456

Separate the public API headers from the mono implementation
Provide a foundation for refactoring the headers (as outlined in [MonoAPI] embedding API testing and API headers quality of life improvements #64456) to be more useful for late binding scenarios

lambdageek · 2022-02-01T04:09:34Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-02-01T04:09:55Z

Azure Pipelines successfully started running 1 pipeline(s).

lambdageek · 2022-02-01T13:35:17Z

/cc @jkotas

jkotas · 2022-02-01T14:19:42Z

src/mono/CMakeLists.txt

@@ -848,6 +848,9 @@ endif()
 ### End of OS specific checks

 include_directories("${CLR_SRC_NATIVE_DIR}")
+
+add_subdirectory("${CLR_SRC_NATIVE_DIR}/include/mono-2.0" mono-2.0)


Do we need the version number in the path? What does the version number mean?

We version the whole runtime together. I think trying to support multiple versions in the same repo would be very complex.

I'm trying to match the layout that the existing runtime pack uses for the headers.

The mono API is always going to be "2.0" as long as its API/ABI compatible.

I'm trying to match the layout that the existing runtime pack uses for the headers.

Where is the existing runtime pack?

https://nuget.info/packages/Microsoft.NETCore.App.Runtime.Mono.iossimulator-x64/6.0.1

The headers are in runtimes/[RID]/native/include/mono-2.0/...

The code that places them there is:

runtime/src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Runtime.props

Lines 69 to 73 in 69b9000

<RuntimeFiles Condition="'$(TargetsMobile)' == 'true'"

Include="@(MonoIncludeFiles)"

ExcludeFromDataFiles="true">

<TargetPath>runtimes/$(RuntimeIdentifier)/native/include/%(RecursiveDir)</TargetPath>

</RuntimeFiles>

(this is also matching the layout that the old mono/mono packages use - include/mono-2.0/mono/{utils,metadata,jit}/...)

I think it would be better to just name them include/metadata etc. and add the extra dirs in the runtime pack creation code.

Perhaps there are still too many mono names. Runtime pack includes Mono in name, then you have monoapi and mono under it.

I'm not sure how much flexibility I have with the names.

In the runtime, we typically have includes like #include <mono/metadata/object.h> so at least one level of mono/ has to be in the repo. In embedding code, the mono-2.0/mono/metadata/object.h directory layout is pretty well established and embedders also typically write #include <mono/metadata/object.h>.

So if I move these headers, in the repo, they need to go some place like src/native/XYZ and beneath that it seems like we must have mono/metadata/object.h.

We could do something likesrc/native/include/mono/metadata/object.h but I don't think that conveys the idea that these headers are not an internal implementation detail of the runtime, but are part of the public API.

What about src/native/public/mono/metadata/object.h? or even src/public/mono/metadata/object.h ?

We have number of other public headers. For example, nethost.h and hostfxr.h (https://docs.microsoft.com/en-us/dotnet/core/tutorials/netcore-hosting).

If the idea is that all public headers that are designed to be included from outside the repo should live in a central place separate from their implementation, we should think about the directory structure for all of these.

If the idea is that all public headers that are designed to be included from outside the repo should live in a central place separate from their implementation, we should think about the directory structure for all of these.

Yea an implementation-independent home for nethost.h and hostfxr.h would be great. (And would make it a bit easier for the mobile hosts to implement the same APIs)

I'm not sure what kind of directory layout would be best. src/native seems more natural than a new toplevel folder under src/ since these are native APIs. But on the other hand src/native is getting crowded.

Personally, I like src/native/public (e.g. src/native/public/mono/metadata/object.h) that you have proposed above.

jkotas · 2022-02-01T14:22:35Z

@joncham Are you going to be able to build on this for what you are trying to do in Unity-Technologies#4 or do you think you will end up with a separate set?

lambdageek · 2022-02-01T14:33:30Z

@joncham Are you going to be able to build on this for what you are trying to do in Unity-Technologies#4 or do you think you will end up with a separate set?

One of the goals of this is to make it easier to refactor the public headers into the MonoTypes.h/MonoFunctions.h sort of layout that Unity have in that PR.

I was thinking that e.g. mono/metadata/object.h would become a roll-up header that includes mono/metadata/details/object-types.h and mono/metadata/details/object-functions.h, etc. The Unity MonoFunctions.h can then just include all the details/*-functions.h headers, while existing code that uses the mono/metadata/*.h headers will keep doing what it does today.

joncham · 2022-02-02T20:01:59Z

I think the overall goals of #64456 make sense and would aid my efforts. The changes mentioned there and openness to supporting late bound (dlopen/dlsym) style clients would be helpful.

lambdageek · 2022-02-11T21:06:08Z

@marek-safar FYI

jkotas · 2022-02-14T16:13:57Z

src/mono/mono/mini/CMakeLists.txt

  install(FILES ${mini_public_headers}
 	DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/mono-2.0/mono/jit)


Why do we still need to install JIT public headers from here?

Yep, that's dead code. Removed it. Thanks!

lambdageek · 2022-02-14T20:10:07Z

Moved things around one more time. Made a public APIs directory in src/native/public with the mono headers directly under public (ie src/native/public/mono/metadata/object.h etc). This just changes the repo layout, the runtime pack layout is still the same (headers under runtimes/[RID]/native/include/mono-2.0 in the nuget)

ghost assigned lambdageek Jan 31, 2022

dotnet-issue-labeler bot added the area-Build-mono label Jan 31, 2022

lambdageek force-pushed the move-mono-include branch 5 times, most recently from db5d7c3 to 432bf4b Compare February 1, 2022 03:43

lambdageek marked this pull request as ready for review February 1, 2022 13:34

lambdageek requested review from BrzVlad, imhameed, lateralusX, marek-safar, naricc, SamMonoRT, thaystg and vargaz as code owners February 1, 2022 13:34

lambdageek requested review from ViktorHofer and akoeplinger February 1, 2022 13:34

jkotas reviewed Feb 1, 2022

View reviewed changes

lambdageek mentioned this pull request Feb 2, 2022

[MonoAPI] embedding API testing and API headers quality of life improvements #64456

Closed

3 tasks

lambdageek force-pushed the move-mono-include branch 2 times, most recently from eec7a5c to 90eddd3 Compare February 7, 2022 14:27

lambdageek changed the title ~~[build] Move Mono API headers to src/native/include/mono-2.0~~ [build] Move Mono API headers to src/native/monoapi Feb 7, 2022

runfoapp bot mentioned this pull request Feb 8, 2022

System.Net.NetworkInformation.Tests.PingTest.SendPingWithHostAndTimeoutAndBuffer failing on OSX #64963

Closed

lambdageek and others added 10 commits February 11, 2022 15:26

Move mono public API utils/ files to src/native/include/

e808d52

Move public mono metadata headers to src/native/include

2f03705

Move the public headers into a mono-2.0 directory

e3517ab

Mono mini public header to src/native/include/mono-2.0

e5b89c0

fix mono dbgshim and mscordbi builds

1e0b463

Pass --nativedir=src/native to offsets-tool.py

97b638d

fix wasm build

31a47c4

fix windows build

19f769f

Add a brief README for the mono-2.0 directory

00b237c

Move mono API to src/native/monoapi

d437227

lambdageek force-pushed the move-mono-include branch from 7d7249d to d437227 Compare February 11, 2022 20:26

lambdageek mentioned this pull request Feb 11, 2022

[tests] Add MonoApi runtime tests #65221

Merged

jkotas reviewed Feb 14, 2022

View reviewed changes

lambdageek added 2 commits February 14, 2022 11:33

mini cmake: remove dead code

b5f97d7

Move everything to src/native/public

eff0a95

lambdageek changed the title ~~[build] Move Mono API headers to src/native/monoapi~~ [build] Move Mono API headers under src/native/public Feb 14, 2022

jkotas approved these changes Feb 14, 2022

View reviewed changes

lambdageek merged commit a85f77a into dotnet:main Feb 15, 2022

lambdageek mentioned this pull request Feb 16, 2022

Add a transport package with libmono-profiler-aot.so on Android #65401

Closed

ghost locked as resolved and limited conversation to collaborators Mar 17, 2022

lambdageek deleted the move-mono-include branch March 19, 2022 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[build] Move Mono API headers under src/native/public #64569

[build] Move Mono API headers under src/native/public #64569

lambdageek commented Jan 31, 2022

lambdageek commented Feb 1, 2022

azure-pipelines bot commented Feb 1, 2022

lambdageek commented Feb 1, 2022

jkotas Feb 1, 2022

lambdageek Feb 1, 2022

jkotas Feb 1, 2022

lambdageek Feb 1, 2022

vargaz Feb 1, 2022

marek-safar Feb 12, 2022

lambdageek Feb 12, 2022 •

edited

Loading

jkotas Feb 14, 2022

lambdageek Feb 14, 2022

jkotas Feb 14, 2022

jkotas commented Feb 1, 2022

lambdageek commented Feb 1, 2022

joncham commented Feb 2, 2022

lambdageek commented Feb 11, 2022

jkotas Feb 14, 2022

lambdageek Feb 14, 2022

lambdageek commented Feb 14, 2022 •

edited

Loading

	<RuntimeFiles Condition="'$(TargetsMobile)' == 'true'"
	Include="@(MonoIncludeFiles)"
	ExcludeFromDataFiles="true">
	<TargetPath>runtimes/$(RuntimeIdentifier)/native/include/%(RecursiveDir)</TargetPath>
	</RuntimeFiles>

		install(FILES ${mini_public_headers}
		DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/mono-2.0/mono/jit)

[build] Move Mono API headers under src/native/public #64569

[build] Move Mono API headers under src/native/public #64569

Conversation

lambdageek commented Jan 31, 2022

lambdageek commented Feb 1, 2022

azure-pipelines bot commented Feb 1, 2022

lambdageek commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Feb 1, 2022

lambdageek commented Feb 1, 2022

joncham commented Feb 2, 2022

lambdageek commented Feb 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek commented Feb 14, 2022 • edited Loading

lambdageek Feb 12, 2022 •

edited

Loading

lambdageek commented Feb 14, 2022 •

edited

Loading