arcade icon indicating copy to clipboard operation
arcade copied to clipboard

Ensure build task projects work when source built

Open JunTaoLuo opened this issue 3 years ago • 41 comments

  • [x] This issue is blocking: source build

There are hardcoded paths in build tasks: https://github.com/dotnet/arcade/blob/0b0d5b90b5aba80c66a9e7a210e0af8093dc4f57/src/Microsoft.DotNet.Build.Tasks.Feed/build/Microsoft.DotNet.Build.Tasks.Feed.targets#L46

This breaks in source built packages where the TFM is different: https://github.com/dotnet/arcade/blob/e7ede87875f41a9b3df898ae08da5ebc96e24f56/eng/TargetFrameworkDefaults.props#L9

JunTaoLuo avatar May 19 '21 19:05 JunTaoLuo

[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.

MattGal avatar May 20 '21 16:05 MattGal

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?

riarenas avatar May 20 '21 20:05 riarenas

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

MattGal avatar May 27 '21 21:05 MattGal

FYI @ViktorHofer how should this issue be triaged?

JunTaoLuo avatar May 29 '21 07:05 JunTaoLuo

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

ViktorHofer avatar May 29 '21 07:05 ViktorHofer

[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)

MattGal avatar Jun 03 '21 19:06 MattGal

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?

ViktorHofer avatar Jun 10 '21 08:06 ViktorHofer

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 avatar Jun 10 '21 15:06 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.

MichaelSimons avatar Jun 10 '21 15:06 MichaelSimons

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.

ViktorHofer avatar Jun 17 '21 09:06 ViktorHofer

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

markwilkie avatar Jun 18 '21 14:06 markwilkie

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.

alexperovich avatar Jul 14 '21 21:07 alexperovich

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 avatar Jul 19 '21 22:07 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...

ChadNedzlek avatar Jul 19 '21 22:07 ChadNedzlek

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.

ilyas1974 avatar Jul 20 '21 17:07 ilyas1974

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.

MichaelSimons avatar Jul 22 '21 00:07 MichaelSimons

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?

alexperovich avatar Jul 22 '21 00:07 alexperovich

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 https://github.com/dotnet/arcade/commit/938b3e8b4edcd96ca0f0cbbae63c87b3f51f7afe for netcoreapp3.1.

ViktorHofer avatar Jul 22 '21 06:07 ViktorHofer

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>

dseefeld avatar Jul 22 '21 13:07 dseefeld

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.

alexperovich avatar Jul 22 '21 19:07 alexperovich

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.

ViktorHofer avatar Jul 27 '21 20:07 ViktorHofer

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?

JunTaoLuo avatar Jul 31 '21 00:07 JunTaoLuo

@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?

ChadNedzlek avatar Aug 05 '21 22:08 ChadNedzlek

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

markwilkie avatar Aug 05 '21 22:08 markwilkie

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

jakubstilec avatar Aug 12 '21 09:08 jakubstilec

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?

ChadNedzlek avatar Aug 26 '21 20:08 ChadNedzlek

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?

ViktorHofer avatar Sep 02 '21 15:09 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?

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.

dseefeld avatar Sep 02 '21 15:09 dseefeld

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

markwilkie avatar Oct 14 '21 16:10 markwilkie

@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?

alexperovich avatar Dec 02 '21 21:12 alexperovich