Ensure build task projects work when source built

[JunTaoLuo]

• This issue is blocking: source build

There are hardcoded paths in build tasks:

arcade/src/Microsoft.DotNet.Build.Tasks.Feed/build/Microsoft.DotNet.Build.Tasks.Feed.targets

Line 46 in 0b0d5b9

<_MicrosoftDotNetBuildTasksFeedTaskDir Condition="'$(MSBuildRuntimeType)' == 'Core'">$(MSBuildThisFileDirectory)../tools/netcoreapp3.1/</_MicrosoftDotNetBuildTasksFeedTaskDir>

This breaks in source built packages where the TFM is different:

arcade/eng/TargetFrameworkDefaults.props

Line 9 in e7ede87

<TargetFrameworkForNETSDK Condition="'$(DotNetBuildFromSource)' == 'true'">net5.0</TargetFrameworkForNETSDK>

[MattGal]

[Async Triage] : I think this has a workaround? (specifying _MicrosoftDotNetBuildTasksFeedTaskDir explicitly on build invocation lets one override the property globally) but it's ugly. If Source Build support for Arcade is under FR's charter I don't think it would be too bad, but I'd defer to @ilyas1974 as to whether that's the case; otherwise don't know where to put it.

[riarenas]

I'm a bit torn on where this should go as well. Seems like either FR, or something the source-build folks could provide guidance for?

[MattGal]

[Async Triage]: No update since my last comment.

[JunTaoLuo]

FYI @ViktorHofer how should this issue be triaged?

[ViktorHofer]

Without fixing this, source building dotnet/runtime and other repos that depend on affected tools will be broken so we should prioritize it correctly.

[MattGal]

[Async Triage]: @dseefeld / @MichaelSimons can you comment on who currently owns this scenario, and what the priorities for making it work are in this situation? (EDIT: Corrected people I was tagging)

[ViktorHofer]

Why does source build not work with the current LTS which is netcoreapp3.1? Aside from that, shouldn't Arcade just upgrade all dependencies to net6.0 so that we have a consistent story across the stack?

[MichaelSimons]

Why does source build not work with the current LTS which is netcoreapp3.1?

Source build always builds with the current sdk to avoid prebuilts.

Aside from that, shouldn't Arcade just upgrade all dependencies to net6.0 so that we have a consistent story across the stack?

Yes that seems to be a source-build friendly solution. It's the only amenable solution that comes to my mind.

[MichaelSimons]

can you comment on who currently owns this scenario, and

Eng Services should own this.

what the priorities for making it work are in this situation?

It is a blocker for source build. The goal is to be able to have a source-build tarball available for preview 6.

[ViktorHofer]

It is a blocker for source build. The goal is to be able to have a source-build tarball available for preview 6.

As some repos already snapped for Preview 6 doesn't that mean that we need to tackle this asap?

Source build always builds with the current sdk to avoid prebuilts.

Right, so currently source build arcade builds for net5.0 and non source build arcade builds for netcoreapp3.1. We should consolidate towards net6.0 and make this a priority.

[markwilkie]

This seems to be blocking source build, so am assigning to FR where it can be triaged.

[alexperovich]

How is source-build re-targeting the projects? Is it just modifying the project to have a different TFM?

Changing the build targets in the packages to work when the TFM is changed is a very non-trivial change. We would need to make the targets files essentially be templates that have the TFM part filled in as part of the build before packing. Considering we have no usable templating task or targets, this change would be rather complex with lots of opportunities for messy bugs. While not impossible, a change like this will further obfuscate the logic in these task packages and make them harder to understand.

[ChadNedzlek]

Based on what Alex says, this sounds like it's not really of a size FR can tackle... we essentially have to build a totally new language/build construct to make source buildable tasks? That's... a lot... it sounds like it needs a design and review and testing and deployment plans...

[ChadNedzlek]

It also sounds like source-build should validate as part of arcade... like it shouldn't be possible for PR's to break source build randomly, and it doesn't sound like there is any sort of validation around that, which is also a ton of work. This sounds like a whole epic...

[ilyas1974]

Based on the comments above, the immediate need that put this into FR has been mitigated (this issue has been open since May and has not been "hot" since then. Based on @ChadNedzlek comments, this is much bigger than FR - sending back to triage.

[MichaelSimons]

How is source-build re-targeting the projects? Is it just modifying the project to have a different TFM?

Yes it builds for a different TFM. As mentioned earlier, it seem like Arcade should just upgrade all dependencies to net6.0 so that we have a consistent story across the stack.

[alexperovich]

I'm asking about the actual mechanics of "build for a different TFM"

what does it actually do to the projects to make this happen?

[ViktorHofer]

We don't have a blocking label, right? Wanted to add one as even though this issue is open since May, it's still blocking sourcebuild to produce 6.0 bits.

Yes it builds for a different TFM. As mentioned earlier, it seem like Arcade should just upgrade all dependencies to net6.0 so that we have a consistent story across the stack.

Exactly that. Arcade should use net6.0 instead of netcoreapp3.1 everywhere to avoid differences in TFMs between a normal and a sourcebuild. The original change that created the divergence wasn't right IMHO. Who owns doing that? Last time the TFM was updated by @alexperovich in 938b3e8 for netcoreapp3.1.

[dseefeld]

I'm asking about the actual mechanics of "build for a different TFM"

what does it actually do to the projects to make this happen?

In source-build, we add patches to build for the current TFM when building from source.

   <PropertyGroup>
     <TargetFrameworks>netcoreapp3.1;net472</TargetFrameworks>
+    <TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">net6.0</TargetFrameworks>
     <ExcludeFromSourceBuild>false</ExcludeFromSourceBuild>
     <PackageType>MSBuildSdk</PackageType>
     <IncludeBuildOutput>false</IncludeBuildOutput>

[alexperovich]

If you are already adding patches then I would suggest just patching the targets files too. Making this work in general is a very large sized work item. This basically requires hooking up build steps that can run a template to produce the targets file output, rather than just including the targets file directly.

[ViktorHofer]

If you are already adding patches

Correct me if I'm wrong but source build doesn't use patches anymore, at least not in dotnet/runtime and other repos that I'm following.

Making this work in general is a very large sized work item. This basically requires hooking up build steps that can run a template to produce the targets file output, rather than just including the targets file directly.

I don't think anyone wants to see a templated approach happening. Instead as already mentioned, we would like to generally bump the TFM used in arcade to net6.0.

[JunTaoLuo]

Any progress on this issue?

Considering we have no usable templating task or targets, this change would be rather complex with lots of opportunities for messy bugs.

Coincidentally, I found this issue while adding the templating task: https://github.com/dotnet/arcade/tree/main/src/Microsoft.DotNet.Build.Tasks.Templating. I understand it's not trivial to use this for all projects which requires a two staged build, this task and then the rest of Arcade, but I'd like to state it as an option.

I like the TFM update to net6.0 but is that feasible (i.e. not breaking)? Will all consumers of these tasks still be able to consume them? This might work in source build since most projects there are net6.0 only but I'm not sure if that's the case for non source-build.

Given 6.0 is in rc already, do we still have runway to do anything here?

[ChadNedzlek]

@markwilkie @mmitche . We have some sort of plan for how we update the required TFM in arcade, correct? It sounds like this needs to be part of that plan?

[markwilkie]

@tkapin - should this be included as part of https://github.com/dotnet/core-eng/issues/13997?

[jakubstilec]

[Async Triage]: It shouldn't be #13997. It's a short epic to handle automatic creation of images in the image factory.

[ChadNedzlek]

If this isn't part of any current funding, and it's not blocking anything... should it be closed? Who needs/wants this? What is driving it's importance?

[ViktorHofer]

This would have helped source build but presumably no one wants to do it. @dseefeld is there a mitigation process in place to avoid the mentioned issue?

[dseefeld]

This would have helped source build but presumably no one wants to do it. @dseefeld is there a mitigation process in place to avoid the mentioned issue?

There is no mitigation for this. In source-build, we need to patch to solve this issue. As Viktor mentions above patching is a temporary solution and we don't want to carry patches over releases, so a fix is needed. Updating to net6.0 would be a good solution for source-build if it is feasible.

[markwilkie]

What's the latest here? We're about to ship, but I haven't heard where this ended up.

[alexperovich]

@dseefeld why exactly is source build changing the tfm? The net6.0 sdk can build netcoreapp3.1 projects.
We would rather not complicate the build unnecessarily. This change is only needed because we are changing the tfm. Can we just.. not change the tfm for at least these task projects?

[tkapin]

@dleeapho - since @dseefeld has left the A&D team, who from your team can work with @alexperovich to answer his question?

[MichaelSimons]

@crummel - can you work w/@alexperovich on this?

[ilyas1974]

@crummel / @alexperovich what is the status of this?

[alexperovich]

I still don't understand why the tfms are getting changed by source-build. The X sdk can build Y projects, given X > Y.

[crummel]

@alexperovich The issue is not the Arcade build itself, the issue is that source-build then uses the Arcade we build in every other Arcade-based repo. Since Arcade bits have to actually run in those downstream builds we need to match the bootstrap runtime that we have, which is the previous version (i.e. when building runtime 5.0.12 we have runtime 5.0.11 available and no other runtimes).

@markwilkie, @ChadNedzlek, and I had a meeting to discuss this. From what Mark was saying, it sounds like the Arcade release/3.1 branch should be targeting netcoreapp3.1, release/5.0 should target net5.0, and so on, but it doesn't look like that's the case. If the branches lined up with the TFMs source-build could upgrade its version of Arcade to an appropriate version that matches the branch. It seems like main also needs to be updated to net7.0 - is this work that is planned on the Arcade side?

Updating the TFMs appropriately would also make #8642 unnecessary.

cc @MichaelSimons

[markwilkie]

cc/ @mmitche for context

We do (by policy only) keep up to date in main w/ the latest SDK. What I'm not sure of is if (or how) we do a final update to the GA version of the SDK when we fork Arcade.

[ViktorHofer]

Keeping the SDK up-to-date is required for what @crummel is asking for but it's only half of what's needed. Even though when a 7.0 SDK is consumed in Arcade, the libraries in the repo still compile against an older tfm, i.e. net5.0. From a policy stand-point we should enforce that the .NETCoreApp tfm being used in Arcade is in sync with the consumed SDK's version:

• release/3.1 should use a 3.0.* SDK and netcoreapp3.1 as the .NETCoreApp tfm.
• release/5.0 should use a 5.0.* SDK and net5.0 as the .NETCoreApp tfm.
• release/6.0 should use a 6.0.* SDK and net6.0 as the .NETCoreApp tfm.
• main should use a 7.0.* SDK and net7.0 as the .NETCoreApp tfm.

Therefore whenever the SDK is updated and its major version changes, the .NETCoreApp tfm should also be updated, preferably in the same PR.

I'm mostly just copying what @crummel said but I wanted to make sure that we are in agreement. In case you want to discuss this further, I'll be back from my extended leave this Friday :)

[mmitche]

@crummel I'm still a little confused about the tfm connection here. A 7.0 SDK doesn't require that we use a 7.0 TFM for all projects. Is source-build forcing a 7.0 TFM everywhere? Why not just build based on what the projects say?

@ViktorHofer This is sometimes not possible, since the tfm usually appears after the major/minor version change.

[ViktorHofer]

@ViktorHofer This is sometimes not possible, since the tfm usually appears after the major/minor version change.

Right, but another principle is to rely on signed builds only which makes it less likely that Arcade would depend on an SDK before it reaches preview1 state (which must include the new tfm).

[tkapin]

@crummel - the last update is from Apr, do you know what's the current state of this issue? Is this something that still needs to be addressed? If so, is this a blocker to shipping the .NET7? Cheers!

[ViktorHofer]

This got fixed with #9127.

[tkapin]

@crummel - can this be closed now?

[ViktorHofer]

Yes, the work is done.