-
Notifications
You must be signed in to change notification settings - Fork 353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure build task projects work when source built #7413
Comments
[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. |
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? |
[Async Triage]: No update since my last comment. |
FYI @ViktorHofer how should this issue be triaged? |
Without fixing this, source building dotnet/runtime and other repos that depend on affected tools will be broken so we should prioritize it correctly. |
[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) |
Why does source build not work with the current LTS which is |
Source build always builds with the current sdk to avoid prebuilts.
Yes that seems to be a source-build friendly solution. It's the only amenable solution that comes to my mind. |
Eng Services should own this.
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?
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. |
This seems to be blocking source build, so am assigning to FR where it can be triaged. |
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. |
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... |
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... |
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. |
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. |
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? |
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.
Exactly that. Arcade should use |
In source-build, we add patches to build for the current TFM when building from source.
|
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. |
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.
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. |
Any progress on this issue?
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? |
@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? |
@tkapin - should this be included as part of https://github.com/dotnet/core-eng/issues/13997? |
[Async Triage]: It shouldn't be #13997. It's a short epic to handle automatic creation of images in the image factory. |
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? |
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. |
What's the latest here? We're about to ship, but I haven't heard where this ended up. |
@dseefeld why exactly is source build changing the tfm? The net6.0 sdk can build netcoreapp3.1 projects. |
@dleeapho - since @dseefeld has left the A&D team, who from your team can work with @alexperovich to answer his question? |
@crummel - can you work w/@alexperovich on this? |
@crummel / @alexperovich what is the status of this? |
I still don't understand why the tfms are getting changed by source-build. The X sdk can build Y projects, given X > Y. |
@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/ @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. |
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:
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 :) |
@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. |
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). |
@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! |
This got fixed with #9127. |
@crummel - can this be closed now? |
Yes, the work is done. |
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
This breaks in source built packages where the TFM is different:
arcade/eng/TargetFrameworkDefaults.props
Line 9 in e7ede87
The text was updated successfully, but these errors were encountered: