Runtime: CoreCLR should error if `clr.buildtools` are not built and you try to build a subset that is dependent of that

Created on 13 Apr 2020  路  8Comments  路  Source: dotnet/runtime

Right now if I do: build clr.runtime I get a native error... something like:

Generating DAC module index file -> C:/repos/runtime/artifacts/obj/coreclr/Windows_NT.x64.Release/src/inc/dacmoduleindex.h
C:\Program Files\dotnet
Specify which project file to use because this 'C:\repos\runtime\artifacts\obj\coreclr\Windows_NT.x64.Release\src\dlls\mscordac' contains more than one project file.
C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(231,5): error MSB6006: "cmd.exe" exited with code 1. [C:\repos\runtime\artifacts\obj\coreclr\Windows_NT.x64.Release\src\d
lls\mscordac\dac_module_index_header.vcxproj]

It turns out this is because we introduced the GetModuleIndex tool which the clr.runtime build depends on. I think we should error out just like we do in other subsets if the clr.buildtools subset is not built and we try to build a dependent subset so that the errors are more clear and easier to fix.

Also, as a side note, since this are local tools, why do we have to build them as a subset manually if we just want to build one piece of coreclr. I think we should follow what we do with other local tools that are built always unless a semaphore file is there:
https://github.com/dotnet/runtime/blob/42183b1b86656a348e0b6af7fa7c75072865f759/Build.proj#L40

cc: @dotnet/runtime-infrastructure

area-Infrastructure-coreclr

Most helpful comment

Using managed tool by the native build is a bad thing due to the fact that native build now depends on the managed one. I have taken a look at the GetModuleIndex tool and it just extracts the build id / uuid / file size and stamp on Linux / OSX / Windows. That can easily be achieved by existing native tools. The readelf -n on Linux, dwarfdump -u on OSX and some .cmd script or simple "C" app executed with try_runon Windows. So it seems it is simple enough so that it would be worth removing the managed dependency.

All 8 comments

Partially a dupe of https://github.com/dotnet/runtime/issues/34912

Also, as a side note, since this are local tools, why do we have to build them as a subset manually if we just want to build one piece of coreclr. I think we should follow what we do with other local tools that are built always unless a semaphore file is there:

Building corelib doesn't need it at all, hence why I believe it makes sense it's still separate. I just think it should be implied by clr.runtime. However, as a workflow for a lot of the native dev's inner loop, overall this is still a pain point as they avoid touching msbuild at all, so they just directly call the build-runtime scripts after building the tools once, which they do using the subset. I do agree that the error is not helpful at all as it stands right now.

Also related: https://github.com/dotnet/runtime/issues/34894

When did this change?

Are you suggesting that the first time you build "clr.runtime" you need to also build "clr.buildtools", but after that you can build "clr.runtime" alone?

Wouldn't it be better to automatically just build "clr.buildtools" if it can be easily/cheaply determined it hasn't been built yet, and is needed? Then, build clr.runtime will always "just work".

However, presumably building "clr.buildtools" is not free, and we want "build clr.runtime" to do the absolute minimum work possible, so if you change what "clr.buildtools" builds (I don't know what that is/means) then you would need to manually specify it?

When did this change?

This was related to single-file diagnostics support changed that went in last Friday.

Are you suggesting that the first time you build "clr.runtime" you need to also build "clr.buildtools", but after that you can build "clr.runtime" alone?

Yes, or use the build-runtime scripts as long as you don't clean artifacts.

Wouldn't it be better to automatically just build "clr.buildtools" if it can be easily/cheaply determined it hasn't been built yet, and is needed? Then, build clr.runtime will always "just work".

Yeah, I share the same opinion, hence why I said "I just think it should be implied by clr.runtime". If someone is using build.sh/cmd they already paid the big cost of such scripts (dotnet acquisition + spinning up msbuild)

However, presumably building "clr.buildtools" is not free, and we want "build clr.runtime" to do the absolute minimum work possible, so if you change what "clr.buildtools" builds (I don't know what that is/means) then you would need to manually specify it?

Right now clr.buildtools is really cheap if you are already using clr.runtime. The big overhead is spinning up msbuild. I think runtime implying this subset would be ok. Thoughts @jkoritzinsky?. We try not to add any managed logic to build-runtime, so that one would be a little more challenging and I do feel like what we should do here is improve the error message.

I think I'd be fine with adding it to the clr.runtime subset in Subsets.props, but I feel strongly that the build-runtime scripts should be the native build only. I want to keep those as slim as possible (and possibly slim them down more in the future) and as thin wrappers around the native build whenever I can. (The fact that is has to generate the native file headers and restore PGO info is something I'd like to move to be done from within MSBuild).

Using managed tool by the native build is a bad thing due to the fact that native build now depends on the managed one. I have taken a look at the GetModuleIndex tool and it just extracts the build id / uuid / file size and stamp on Linux / OSX / Windows. That can easily be achieved by existing native tools. The readelf -n on Linux, dwarfdump -u on OSX and some .cmd script or simple "C" app executed with try_runon Windows. So it seems it is simple enough so that it would be worth removing the managed dependency.

I volunteer making such a change unless someone else want to do so.

cc: @noahfalk @mikem8361

I really don't like that we can't use managed build tools (with the resulting SDK dependency), but if this is an absolute requirement, then we could turn off the FEATURE_SINGLE_FILE_DIAGNOSTICS by default. But we will need it back on as soon as the superhost is implemented.

@janvorli if you want to write the scripts (which probably will take longer than it took me to write GetModuleIndex since I leveraged fairly well tested ELF, Windows and MachO parsers), go ahead. I'm not fluent in awk/sed to parse the readelf/dwarfdump/dumpbin? output and it would take me a long time to get it right.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

omariom picture omariom  路  3Comments

noahfalk picture noahfalk  路  3Comments

EgorBo picture EgorBo  路  3Comments

sahithreddyk picture sahithreddyk  路  3Comments

bencz picture bencz  路  3Comments