Skip to content
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

Merged
merged 5 commits into from
Apr 9, 2019
Merged

Update KoreBuild + SDK #9033

merged 5 commits into from
Apr 9, 2019

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Apr 3, 2019

No description provided.

@Eilon Eilon added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Apr 3, 2019
@Eilon Eilon requested a review from dougbu April 3, 2019 17:03
Copy link
Member

@dougbu dougbu left a 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

@pranavkm pranavkm force-pushed the prkrishn/update-sdk branch 2 times, most recently from be01c9c to a141f4b Compare April 6, 2019 00:14
@pranavkm pranavkm marked this pull request as ready for review April 6, 2019 00:20
@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 6, 2019

@dougbu does this look familiar:

F:\workspace\_work\1\s\build\repo.targets(138,5): error MSB4018: The "RepoTasks.RemoveSharedFrameworkDependencies" task failed unexpectedly. [F:\workspace\_work\1\s\.dotnet\buildtools\korebuild\3.0.0-build-20190405.1\KoreBuild.proj]
F:\workspace\_work\1\s\build\repo.targets(138,5): error MSB4018: System.InvalidOperationException: The element 'metadata' in namespace 'http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd' has invalid child element 'frameworkReferences' in namespace 'http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd'. List of possible elements expected: 'packageTypes, releaseNotes, frameworkAssemblies, title, language, developmentDependency, contentFiles, references, summary, license' in namespace 'http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd'. This validation error occurred in a 'frameworkReferences' element. [F:\workspace\_work\1\s\.dotnet\buildtools\korebuild\3.0.0-build-20190405.1\KoreBuild.proj]
F:\workspace\_work\1\s\build\repo.targets(138,5): error MSB4018:    at NuGet.Packaging.Manifest.<>c.<ValidateManifestSchema>b__20_0(Object sender, ValidationEventArgs e) [F:\workspace\_work\1\s\.dotnet\buildtools\korebuild\3.0.0-build-20190405.1\KoreBuild.proj]
F:\workspace\_work\1\s\build\repo.targets(138,5): error MSB4018:    at System.Xml.Schema.XNodeValidator.ValidationCallback(Object sender, ValidationEventArgs e) [F:\workspace\_work\1\s\.dotnet\buildtools\korebuild\3.0.0-build-20190405.1\KoreBuild.proj]

@dougbu
Copy link
Member

dougbu commented Apr 6, 2019

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

@JunTaoLuo
Copy link
Contributor

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?

@dougbu
Copy link
Member

dougbu commented Apr 6, 2019

Hmm, maybe but only if NuGet made a breaking change while adding the new NuGet/Home#7342 feature.

@pranavkm pranavkm closed this Apr 8, 2019
@pranavkm pranavkm reopened this Apr 8, 2019
@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 8, 2019

cc @natemcmaster

Bumping up NuGet.Build.Tasks to 5.0.0 didn't help. frameworkReferences is a fairly new feature, do we need to reference a preview build of NuGet in repo-tasks?

@JunTaoLuo
Copy link
Contributor

cc @rrelyea @nkolev92? Can you comment on the NuGet requirements for frameworkReferences support?

@natemcmaster
Copy link
Contributor

Which package is being processed when the error about frameworkReferences is thrown? We haven't actually started using <FrameworkReference> to build our own packages (#4257), so I'm surprised this has appeared in generated nuspec.

@nkolev92
Copy link

nkolev92 commented Apr 8, 2019

As far as I know, the FrameworkReference pack support is not yet merged anywhere.
It'll be in the 5.1.0 NuGet release, but it will still considered a preview feature.

I think understanding @natemcmaster's point is key.

@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 8, 2019

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

@JunTaoLuo
Copy link
Contributor

JunTaoLuo commented Apr 8, 2019

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.

@JunTaoLuo
Copy link
Contributor

JunTaoLuo commented Apr 8, 2019

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 Nuget.Packaging.Manifest.ReadFrom API. Seems like it's related to the SDK update.

@natemcmaster
Copy link
Contributor

As far as I know, the FrameworkReference pack support is not yet merged anywhere.

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. May require adding a new restore source to https://dotnet.myget.org/F/nuget-build/api/v3/index.json (cref https://github.com/dotnet/toolset/blob/release/3.0.1xx/eng/Versions.props#L55)

@nkolev92
Copy link

nkolev92 commented Apr 8, 2019

5.1.0-rtm-5921 is the first insertion into the CLI/SDK that has the FrameworkReference work as far as NuGet is concerned.
And the PR to merge that is on hold dotnet/cli#11054

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.
Would this have anything to do with that? dotnet/toolset#593

@@ -1,6 +1,6 @@
{
"sdk": {
"version": "3.0.100-preview4-010309"
"version": "3.0.100-preview4-011136"
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
dotnet/toolset#593 (comment)

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

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor

@JunTaoLuo JunTaoLuo Apr 8, 2019

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 are changes in the WebSDK that are require to the test the preview4 templates.

@nguerrera
Copy link
Contributor

And the PR to merge that is on hold dotnet/cli#11054

That's for 2.1.7xx

For 3.0, it was dotnet/toolset#593

@nkolev92
Copy link

nkolev92 commented Apr 8, 2019

@nguerrera I think regardless of whether it gets reverted or not, the SDK should set "pack=false" to Microsoft.NETCore.App.

@nguerrera
Copy link
Contributor

@nguerrera I think regardless of whether it gets reverted or not, the SDK should set "pack=false" to Microsoft.NETCore.App.

@dsplaisted

@JunTaoLuo
Copy link
Contributor

@pranavkm Looks like it's fixed. Feel free to proceed with QB process.

@JunTaoLuo JunTaoLuo requested a review from dougbu April 9, 2019 01:42
@pranavkm pranavkm changed the base branch from master to release/3.0-preview4 April 9, 2019 16:08
@natemcmaster
Copy link
Contributor

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?

@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 9, 2019

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

@Eilon
Copy link
Member

Eilon commented Apr 9, 2019

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.

@pranavkm pranavkm changed the base branch from release/3.0-preview4 to master April 9, 2019 17:48
@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 9, 2019

Updated to point this to master.

@pranavkm pranavkm merged commit 44b8fbc into master Apr 9, 2019
@pranavkm pranavkm deleted the prkrishn/update-sdk branch April 9, 2019 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants