Make DotNetBuildFromSource work in more cases.

[tmds]

DotNetBuildFromSource is mostly used when this repository is built as part of source-build.

With these changes the flag works in more cases, making it easier to reproduce the source-build behavior when building the runtime repository directly.

I came to these changes by fixing the errors that came up while running the following command on my Fedora machine:

./build.sh --runtimeconfiguration Release --librariesConfiguration Debug --subset clr+libs+libs.tests --test /p:DotNetBuildFromSource=true /p:RuntimeOS=linux

cc @MichaelSimons @ViktorHofer @omajid

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[omajid]

Would it make sense to drop some now-duplicated configuration from

runtime/eng/SourceBuild.props

Lines 35 to 54 in 842ec4a

	<InnerBuildArgs>$(InnerBuildArgs) --arch $(TargetArch)</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) --configuration $(Configuration)</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) --allconfigurations</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) --verbosity $(LogVerbosity)</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) --nodereuse false</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) --warnAsError false</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) --outputrid $(TargetRid)</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) --portablebuild $(SourceBuildPortable)</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) /p:NoPgoOptimize=true</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) /p:KeepNativeSymbols=true</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) /p:RuntimeOS=$(RuntimeOS)</InnerBuildArgs>
	<InnerBuildArgs Condition="'$(OfficialBuildId)' != ''">$(InnerBuildArgs) /p:OfficialBuildId=$(OfficialBuildId)</InnerBuildArgs>
	<InnerBuildArgs Condition="'$(ContinuousIntegrationBuild)' != ''">$(InnerBuildArgs) /p:ContinuousIntegrationBuild=$(ContinuousIntegrationBuild)</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) /p:BuildDebPackage=false</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) /p:EnableNgenOptimization=false</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) /p:AdditionalRuntimeIdentifierParent=$(BaseOS)</InnerBuildArgs>
	<InnerBuildArgs>$(InnerBuildArgs) /p:EnablePackageValidation=false</InnerBuildArgs>
	<!-- Re-enable for source-build when 8.0 Preview 1 shipped. See https://github.com/dotnet/installer/pull/14991 for more information. -->
	<InnerBuildArgs>$(InnerBuildArgs) /p:ApiCompatValidateAssemblies=false</InnerBuildArgs>
	<InnerBuildArgs Condition="'$(SourceBuildUseMonoRuntime)' == 'true'">$(InnerBuildArgs) /p:PrimaryRuntimeFlavor=Mono /p:RuntimeFlavor=Mono</InnerBuildArgs>

?

[tmds]

Would it make sense to drop some now-duplicated configuration from runtime/eng/SourceBuild.props

@omajid I don't know. That file doesn't actually set DotNetBuildFromSource.
@MichaelSimons is it safe to remove them?

[omajid]

AFAIK, arcade sets both DotNetBuildFromSource and loads up that file. From a source-build/VMR perspective, the two are tied together.

[tmds]

AFAIK, arcade sets both DotNetBuildFromSource and loads up that file. From a source-build/VMR perspective, the two are tied together.

Source-build sets DotNetBuildFromSource using an envvar.

I don't know if it is possible to use eng/SourceBuild.props outside of source-build that would cause it to not be set.
Or if we care about that.

If someone confirms it isn't, I'll remove the properties from the SourceBuild.props file.

[tmds]

@MichaelSimons can you review the defaults being added here when DotNetBuildFromSource is true, and answer the question whether we can remove these properties from eng/SourceBuild.props?

[MichaelSimons]

Building with DotNetBuildFromSource is not the right way to build at the repo level. ArcadeBuildFromSource is the correct flag to use. The intended dev UX to build a repo in the source-build configuration is to use ./build.sh -sb

@MichaelSimons can you review the defaults being added here when DotNetBuildFromSource is true, and answer the question whether we can remove these properties from eng/SourceBuild.props?

IIRC there was a preference to try and localize all the source-build properties in SourceBuild.props rather than conditionalize them throughout the code base as it made it easier to determine the source-build configuration/settings.

I have no strong preference and will leave that decision to the runtime folks. If these properties are defined, then yes I agree they should be removed from SourceBuild.props.

[tmds]

Building with DotNetBuildFromSource is not the right way to build at the repo level. ArcadeBuildFromSource is the correct flag to use. The intended dev UX to build a repo in the source-build configuration is to use ./build.sh -sb

I think ArcadeBuildFromSource is a good option if you want to build similar to how it happens as part of source-build.

Besides that use-case, I think it's desired to be able to directly use ./build.sh while developing, running tests, etc.

ArcadeBuildFromSource adds a layer of abstraction that isn't helping. For example, it clones the sources under artifacts, and controls ./build.sh arguments.

I think DotNetBuildFromSource can provide something closer to the regular development experience. That is what I'm aiming for with these changes.

[ViktorHofer]

Great progress!

How do we guarantee that devs don't add new stuff to SourceBuild.props for non entry-point switches / properties? Should we add a big disclaimer to that file after the switches are passed in? What about the remaining non entrypoint properties that are still defined in the file?

[tmds]

How do we guarantee that devs don't add new stuff to SourceBuild.props for non entry-point switches / properties? Should we add a big disclaimer to that file after the switches are passed in? What about the remaining non entrypoint properties that are still defined in the file?

Let's look at what is still there:

      <InnerBuildArgs>$(InnerBuildArgs) --arch $(TargetArch)</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) --configuration $(Configuration)</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) --allconfigurations</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) --verbosity $(LogVerbosity)</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) --nodereuse false</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) --warnAsError false</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) --outputrid $(TargetRid)</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) /p:KeepNativeSymbols=true</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) /p:RuntimeOS=$(RuntimeOS)</InnerBuildArgs>
      <InnerBuildArgs Condition="'$(OfficialBuildId)' != ''">$(InnerBuildArgs) /p:OfficialBuildId=$(OfficialBuildId)</InnerBuildArgs>
      <InnerBuildArgs Condition="'$(ContinuousIntegrationBuild)' != ''">$(InnerBuildArgs) /p:ContinuousIntegrationBuild=$(ContinuousIntegrationBuild)</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) /p:BuildDebPackage=false</InnerBuildArgs>
      <InnerBuildArgs>$(InnerBuildArgs) /p:AdditionalRuntimeIdentifierParent=$(BaseOS)</InnerBuildArgs>
      <InnerBuildArgs Condition="'$(SourceBuildUseMonoRuntime)' == 'true'">$(InnerBuildArgs) /p:PrimaryRuntimeFlavor=Mono /p:RuntimeFlavor=Mono</InnerBuildArgs>

The properties that pass down arguments from the source-build top-level can stay.
Also properties that control the output artifacts (--allconfigurations).
Or improve robustness (` --warnAsError false --nodereuse false).

So from this list, I think we still want to move KeepNativeSymbols and BuildDebPackage.

I'm not sure we need to set BuildDebPackage, as there seems nothing that sets it to true?

I'll add a comment that suggests properties that control the source-build build configuration should be added to the repository using the DotNetBuildFromSource Condition.

I ran the command I was using before again, and I noticed my target rid is linux-x64, though I expect it to be fedora.37-x64. I think the build configuration is appropriate, but it got named badly. I will look into that first.

[ViktorHofer]

I'm not sure we need to set BuildDebPackage, as there seems nothing that sets it to true?

This YML template is the only place that enables it:

runtime/eng/pipelines/installer/jobs/build-job.yml

Line 29 in 2ca7cf7

packagingArgs: /p:BuildDebPackage=true

[tmds]

This YML template is the only place that enables it:

I'm assuming it's safe to just remove this and I don't have to add it anywhere unless CI will tell me otherwise.

[tmds]

I verified this command works as expected:

./build.sh --runtimeconfiguration Release --librariesConfiguration Debug --subset clr+libs+libs.tests --test /p:DotNetBuildFromSource=true

I don't have any more changes in mind.

[tmds]

The CI failures seem unrelated.

@ViktorHofer can you take a look at the latest changes?

[ViktorHofer]

Thanks a lot 👍

[tmds]

@ViktorHofer thanks for your help!

[uweigand]

This commit seems to have broken builds where --runtimeconfiguration is not specifically set on the command line:

/home/uweigand/.nuget/packages/microsoft.net.sdk.il/8.0.0-preview.2.23112.4/targets/Microsoft.NET.Sdk.IL.targets(136,5): error MSB3073: The command ""/home/uweigand/runtime/artifacts/bin/coreclr/linux.s390x./ilasm" -QUIET -NOLOGO -DLL  -OUTPUT="/home/uweigand/runtime/artifacts/obj/TestILAssembly/Debug/netstandard2.0/TestILAssembly.dll" -KEY="/home/uweigand/.nuget/packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.23115.1/tools/snk/MSFT.snk" TestILAssembly.il" exited with code 127. [/home/uweigand/runtime/src/libraries/Common/tests/System/TestILAssembly/TestILAssembly.ilproj]

Note how it is trying to use coreclr/linux.s390x./ilasm because $(RuntimeConfiguration) is still empty here:

-    <CoreCLRToolPath>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'coreclr', '$(TargetOS).$(TargetArchitecture).$(Configuration)'))</CoreCLRToolPath>
+    <CoreCLRToolPath>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'coreclr', '$(TargetOS).$(TargetArchitecture).$(RuntimeConfiguration)'))</CoreCLRToolPath>

That property is computed a few lines down in the Directory.Build.props file, but seems to not yet be available here.