search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.39k stars 4.75k forks source link

macOS build fails on NativeRuntimeEventSource.cs with old python version #688

Closed marek-safar closed 3 years ago

marek-safar commented 4 years ago

Initial cost estimate: 2 days Initial contacts: @trylek, @ViktorHofer

macOS build fails when old python is used without an easy way to see that. I have the default python installed by macOS (2.7) but that's not enough to build runtime master version. The build fails with following

CSC : error CS2001: Source file '/<path>/runtime/src/coreclr/../../artifacts/obj/coreclr/System.Private.CoreLib/x64/Debug/../../../Eventing/x64/Debug/NativeRuntimeEventSource.cs' could not be found.

The error is gone after installing python3.

hoyosjs commented 4 years ago

@sywhang just fyi

jashook commented 4 years ago

Python3 is a requirement, if we do not have an error message this issue should track adding one.

ViktorHofer commented 3 years ago

@jkotas do you think it would make sense to convert the https://github.com/dotnet/runtime/blob/15460311833f161bbe90b61ef94a8f339178840b/src/coreclr/src/scripts/genRuntimeEventSources.py script which runs during the CoreLib build to msbuild (probably via a C# msbuild task)?

jkotas commented 3 years ago

Yes!!

I think we should be eliminating our dependencies on python, first in build, and eventually in tests too.

trylek commented 3 years ago

@ViktorHofer - in light of JanK's response please file a follow-up CoreCLR infra item to remove the Python script by rewriting it probably to C# per your suggestion. I guess that in such case it doesn't make too much sense to continue pursuing this work item. We'll need to update the CoreCLR infra backlog local dev innerloop epic to reflect the follow-up item, sadly this is still very much manual.

ViktorHofer commented 3 years ago

Filed https://github.com/dotnet/runtime/issues/45733. Let's close this one then.

trylek commented 3 years ago

Thanks Viktor, I have updated the backlog as appropriate. Feel free to adjust the initial cost estimate, just please let me know or update the dev innerloop epic in the infra backlog to reflect the change.

ViktorHofer commented 3 years ago

I don't think this should take longer than a week max. I wonder if @hoyosjs or @jkoritzinsky might be interested in picking this task up.

hoyosjs commented 3 years ago

There's three scripts, it needs to be validated for ETW, LTTNG, and EventPipe. There's different behavior for each and different codegen for each. A week isn't realistic. I really don't know how much value there's to this right now. Diagnostics is pretty swamped on work and issues at the moment. We had considered this at some point, but just didn't see enough value on the time investment.

cc: @dotnet/dotnet-diag

ViktorHofer commented 3 years ago

There's three scripts, it needs to be validated for ETW, LTTNG, and EventPipe. There's different behavior for each and different codegen for each. A week isn't realistic.

Got it. Reminds me that I shouldn't estimate work for an area I'm not familiar with 🗡️

ViktorHofer commented 3 years ago

Regarding impact, besides the overall goodness of one less build dependency, this lowers the first time contributor bar noticeably. Based on issues we've seen in the past, python is one of the tools that isn't installed on every dev's machine.