-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Update KoreBuild + SDK #9033
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
Update KoreBuild + SDK #9033
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
global.json should also change to match the underlying SDK
be01c9c
to
a141f4b
Compare
@dougbu does this look familiar:
|
@pranavkm I don't recall seeing that error before. Is it already described in an AspNetCore or AspNetCore-Internal issue? @JunTaoLuo do you have an inkling what's going wrong w/ that SharedFx task? |
Hmm not sure. Though I'm wondering if it's related to NuGet/Home#7342? It's a new feature that just got merged so maybe we need a new version of NuGet? |
Hmm, maybe but only if NuGet made a breaking change while adding the new NuGet/Home#7342 feature. |
a141f4b
to
a66515b
Compare
Bumping up NuGet.Build.Tasks to 5.0.0 didn't help. |
Which package is being processed when the error about |
As far as I know, the FrameworkReference pack support is not yet merged anywhere. I think understanding @natemcmaster's point is key. |
@JunTaoLuo would you be able to drive investigating the source of the problem? FYI @Eilon since this is blocking some template work (#6392) that requires a fairly new WebSdk. |
Hmm looks like Microsoft.AspNetCore.ApiAuthorization.IdentityServer failed the task. Looking into why. EDIT: Actually that's just the first package, whatever the problem is, it's hitting all of our packages probably. |
The packages we build has the following in the nuspec: <frameworkReferences>
<group targetFramework=".NETCoreApp3.0">
<frameworkReference name="Microsoft.NETCore.App" />
</group>
</frameworkReferences> This is probably what's confusing the |
It must be if the nuspec generated by the SDK has a framework reference in it. @pranavkm try updating to NuGet.Build.Tasks |
5.1.0-rtm-5921 is the first insertion into the CLI/SDK that has the FrameworkReference work as far as NuGet is concerned. Which version of the SDK is the one that's creating that nuspec? I'm not sure how the nuget bits got ingested to cause breakages. |
@@ -1,6 +1,6 @@ | |||
{ | |||
"sdk": { | |||
"version": "3.0.100-preview4-010309" | |||
"version": "3.0.100-preview4-011136" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkolev92 this is the version of the SDK we're trying to update to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nguerrera We're seeing this feature show up in P4 builds of the SDK...did something changed in the last 6 days since this comment, or did this unintentionally get merged into P4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the feature being present break things?
I intentionally did not revert it because I figured we should get coverage of latest bits. There was no expectation of anything breaking. I chatted briefly with @nkolev92 after that comment.
If necessary, we can revert the nuget version in P4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that @pranavkm updated to the latest SDK available as of today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. The question is why is taking a nuget with the frameworkReferences feature a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It breaks the aspnet build because we have some custom MSBuild tasks reading the nuspec, and NuGet code throws when unrecognized elements appear in the metadata.
FWIW, though, the reason we have these custom MSBuild tasks was to workaround the absence of this NuGet feature...so having access to this SDK unblocks us to start using <FrameworkReference>
the right way.
@pranavkm was there any particular reason you were trying to update to the latest SDK? If not, maybe we should combine this update with a solution for #4257
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, essentially there's a mismatch in the toolset which uses nuget 5.1.0-rtm.5921 to build our packages and the NuGet.Build.Tasks that we use to post process the packages built. To reduce the work that's required to update the SDK, which to my understanding is blocking us, why don't we just update the version of NuGet.Build.Tasks that we use to 5.1.0-rtm.5921 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's for 2.1.7xx For 3.0, it was dotnet/toolset#593 |
@nguerrera I think regardless of whether it gets reverted or not, the SDK should set "pack=false" to Microsoft.NETCore.App. |
|
@pranavkm Looks like it's fixed. Feel free to proceed with QB process. |
I'm a little concerned about accepting this change so late in the branching process. It sounds like there was a lot for churn in the SDK w.r.t. framework references. Before we accept this, can you try using some of the netcoreapp3.0 packages produced by this build to make sure they still work as expected? |
@natemcmaster I agree, it does seem a bit risky at this point. @danroth27 is considering not taking the template change at this point, so we might be happy taking this update in master instead. |
OK if there's too much risk and we're not desperate for this change, then let's just do 'master' and let the bits settle. |
Updated to point this to master. |
No description provided.