-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Move EventPipe C library into shared location. #44791
Move EventPipe C library into shared location. #44791
Conversation
I am not sure whether we need the second |
LGTM otherwise. Thank you! |
The name we have discussed before on Teams was src/shared. Either src/shared or src/common sound good to me. We have prior art for both. |
Totally agree, will drop the second |
Actually, I am thinking whether the best name for this would be For example, we can move the host under this directory from under installer where it does not really belong (related to #43700 - cc @VSadov @vitek-karas). It would look a bit odd for the host to be |
In my opinion I like Also we have System.Globalization.Native native shim under |
With this plan, I think we can consider moving everything under src/libraries/native to src/native |
Great, sounds like we align on I will do change tomorrow morning, so there is still some time to change our minds 😊. |
Seconding CC @dotnet/dotnet-diag - this is a good PR for catching up on the EventPipe x Mono workstream. it highlights the shared components, but is also a good jump point for identifying the runtime specific code. |
@josalem I have a separate PR coming up after this one, implementing the CoreClr shim + add it into CoreClr build (initially as an opt in feature). With that PR we can build CoreClr with either C++ or C version of EventPipe for sometime while transitioning over to new library. |
I also really like the |
As for the plan for the native libs - on an example of I'd mildly prefer the I also assume we are not moving S.G.N right away, just thinking one step ahead? |
Common parts could also be plain code files (.c,.cpp) included by make file elsewhere in the tree. There was a wish to share some code files across varied (high-level) components of the repo. One example is a method to |
So for the eventpipe library (the one moved in this PR), should we do |
By "native libraries" I mean native parts of FX libraries - like From that point eventpipe is a separate thing, so it would be |
Sounds good to me.
I would keep it as |
Why would it be messy? As you have said, it is a few different libraries. I do not see very fundamental difference between eventpipe and the other In any case, we do not have to solve this now. |
We have native code common to libraries (like zlib) or to Unix only (i.e. PAL stuff) and thus the code is currently organized into a subtree like https://github.com/dotnet/runtime/tree/master/src/libraries/Native . It probably makes sense to keep it in that shape. I am less familiar with Yes, no need to solve this now. Not a big deal anyways. |
Moved runtime agnostic C EventPipe/DiagnosticServer library to When all CI checks have passed I believe this PR is ready to go! |
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.
Thank you!
I like this - would definitely make more sense than the current location. Just curious as I didn't see this explicitly stated above - the plan is to move tests for the native event pipe under It would also be good to think about what the tests are "Allowed" to do in this subtree - for example all the hosts tests are in C# (with only small parts in native). Maybe that's OK... |
The tests I called out above is primarily low level native EventPipe unit tests developed when doing the transform of C++ CoreClr EventPipe/DiagnosticServer library into a shareable component. Those tests are currently hosted under src/mono/mono/eventpipe/test, since they are depending on mono eglib test runner and some mono runtime API's. My idea was to make them platform agnostic, so we can at least share the test code (might be a runtime specific piece and build connected to the test runner) somewhere under src/native as well, but that is for later. |
I think In fact, we do have a bunch of eventpipe tests there already: https://github.com/dotnet/runtime/tree/master/src/tests/tracing/eventpipe . |
Jupp there are a bunch of managed EventPipe/DiagnosticServer tests under |
Move C version of EventPipe library into a folder that can be shared by both CoreClr and Mono:
src/native/eventpipe
All runtime specific EventPipe files (shim implementation, build files etc) will be kept under each runtime (mono/mono/eventpipe for example). Example on how shared EventPipe sources will be consumed by mono build, src/mono/mono/eventpipe/CMakeLists.txt.
PR also adds ability to build mono/mono/eventpipe/test using CMake (needed after mono switched to cmake). Tests are still mono specific so will be kept there for now, but will most likely be shared between runtimes at a later point in time.
@jkotas @stephentoub Do you believe the location of new folder where we can share native code between runtimes is OK, src/native/eventpipe?
@lambdageek @josalem