Adding to PackageIndex all of the information about our release from 6.0 and fixing packages that needed placeholders by joperezr · Pull Request #27385 · dotnet/corefx

joperezr · 2018-02-22T22:00:44Z

Fixes #27100

After shipping UAP 6.0 we didn't update the package index with this data so package validation has not been testing for breaks on packages that we will soon release. These changes include updating said index and reacting to the validation errors.

cc: @weshaggard @ericstj @joshfree

joshfree · 2018-02-22T22:45:16Z

can you PR this to master first instead of release/2.1?

ericstj · 2018-02-22T22:50:06Z

+    <PackageReference Include="Microsoft.NETCore.UniversalWindowsPlatform">
+      <Version>$(UAPPackageVersion)</Version>
+    </PackageReference>
+    <PackageReference Include="System.Reflection.Emit">


Please add a comment around these additions and their purpose.

ericstj · 2018-02-22T22:53:07Z

      netcoreapp2.0;
+      uap10.0.16299-Windows_NT;
+      uap10.0.16299aot-Windows_NT;
      uapaot-Windows_NT;


Following the netcoreapp2.0 pattern perhaps you should remove the version-less ones into BuildConfigurations.

sounds good, I'll do that.

ericstj · 2018-02-22T22:54:13Z

  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <PropertyGroup>
    <ProjectGuid>{7DF3C428-AAD6-41C7-98E6-6CACFD5C391E}</ProjectGuid>
+    <PackageTargetFramework>netstandard2.0;uap10.0.16299</PackageTargetFramework>


Shouldn't the NS2.0 version apply or are we still missing a NuGet.Frameworks update?

I believe we are still missing an update of the mappings.

I would expect this mapping to exist as well.

joperezr · 2018-02-23T00:56:59Z

@joshfree

can you PR this to master first instead of release/2.1?

That was the original idea, but because we have turned off all of the uap build in there, it was impossible to get the right validation since the builds were incomplete.

weshaggard · 2018-02-23T01:05:37Z

    <_packageRID Condition="'$(PortableBuild)' == 'true'">$(_portableOS)-$(ArchGroup)</_packageRID>
-    <_packageRID Condition="'$(TargetGroup)' == 'uap'">win10-$(ArchGroup)</_packageRID>
-    <_packageRID Condition="'$(TargetGroup)' == 'uapaot'">win10-$(ArchGroup)-aot</_packageRID>
+    <_packageRID Condition="$(TargetGroup.StartsWith('uap'))">win10-$(ArchGroup)</_packageRID>


Why this change?

Nevermind I see the other aot configurations.

weshaggard · 2018-02-23T01:10:40Z

@@ -811,11 +826,13 @@
        "4.2.1.0": "4.5.0"
      }
    },
+    /*The shim of System.ComponentModel.Composition shipped on netcoreapp2.0 and uap6.0 and we now have an implementation


Would you mind fixing this while you are in here.

weshaggard · 2018-02-23T20:41:34Z

      "InboxOn": {
        "monoandroid10": "Any",
        "monotouch10": "Any",
+        "netcoreapp2.0": "Any",


You can remove the comment now.

…6.0 and fixing packages that needed placeholders

… accordingly

joperezr · 2018-02-27T21:08:34Z

@weshaggard & @ericstj do you mind taking another peak at this before merging?

weshaggard · 2018-02-27T21:54:05Z

        System.Security.Principal.Windows;
      </Value>
    </ValidatePackageSuppression>
+    <!-- Permit missing Inbox since we this used to be a shim and it is now OOB -->


typo: since we this -> since this

weshaggard · 2018-02-27T21:54:29Z

        System.Private.Reflection.Metadata.Ecma335;
      </Value>
    </ValidatePackageSuppression>
+    <!-- Permit missing Inbox since we this used to be a shim and it is now OOB -->


typo: since we this -> since this

weshaggard · 2018-02-27T21:54:59Z

  <PropertyGroup>
    <PackageConfigurations>
      netstandard;
+      netstandard-Windows_NT;


Why do we need this netstandard-Windows_NT configuration?

Because since we harvest a Windows specific netstandard1.3, then package validation will complain given that the compile asset resolved for UAP6.0 (the netstandard2.0 ref) will have a higher assembly version than the resolved implementation for UAP6.0 (the windows specific netstandard1.3 implementation) because the runtime specific assets will be prefered over the non rid specific (lib/netstandard2.0) . This is required in order to pass package validation and in order to be consumed on UAP.

@ericstj do you think adding the netstandard-Windows_NT configuration is better then remapping the harvested location to put the NS13 asset in the lib instead of the windows RID. I suppose there is a chance that remapping the path might actually cause breaks but it would depend on the actual package.

That ns1.3 asset is a windows specific implementation. If you started putting that in a RID-less folder you'd change the behavior for non-windows (previously it got a PNS exception).

If you go back and look at the history as to why this is targeting NETCoreapp-window_nt today instead of netstandard-windows_nt, it is because of this: joperezr@28097ca#diff-6b3a566cbc8f4a17c79fa82c0a0feafc. I needed to create a netcoreapp configuration to avoid an assembly cycle in the implementation of netcoreapp.

In that case the better fix here would be to move the netcoreapp-* configurations into BuildConfigurations only (since their only difference from netstandard version will be referenced assemblies) and have a netstandard-windows_NT version in the package.

weshaggard · 2018-02-27T21:55:37Z

  <PropertyGroup>
  <PackageConfigurations>
    netstandard;
+    netstandard-Windows_NT;


Why do we need this configuration?

Same reason as above.

weshaggard · 2018-02-27T21:56:00Z

  <PropertyGroup>
    <PackageConfigurations>
      netstandard;
+      netstandard-Windows_NT;


Why is this needed? In general I like to avoid RID netstandard configurations.

I agree, but as explained above, this is required because we are harvesting a windows specific netstandard1.3 implementation for this package, which will be prefered over the lib/netstandard2.0 implementation for UAP.

@weshaggard why do you want to avoid RID-netstandard configurations? What if we want to share a windows implementation between netcoreapp and UAP? That's the reason why those old harvested assets existed that were RID-specific and targeted netstandard.

Maybe it is the purity in me speaking but I just don't like mixing netstandard library assets, which should be simple and free (mostly) of any OS specific code, with a RID. While I think there might be exceptions I would generally push for keeping RID specific assets targeting a concrete framework.

There is nothing impure about having a RID + netstandard assets. Consider that the netstandard version targeted by the assembly represents the managed API needed by that assembly and the RID represents the platform API/behavior. It's actually more pure to have the assembly be win+netstandard since that's technically all it depends on. Making it win+UAP / win+netcoreapp implies it depends on something specific to those TFMs which at least the package asset does not.

By purity I'm thinking about netstandard being pure IL and no pinvokes, which is where I would like most libraries to live. I know that isn't a reality but it is my ideal world. I agree using a combination of netstandard+RID can be useful in some scenarios but it also adds some risk as the native dependencies may or may not work on any netstandard supported platform. I guess that is true for all assets that go under a RID and I've never really liked how "undefined" RID's are which also leads me to not liking these. At any rate netstandard+RID is a tool and we should use it if necessary but that doesn't make me like it :)

weshaggard · 2018-02-27T22:28:45Z

@joperezr we are going to be merging master into release/2.1 later this week. It might be worth waiting on this until after that otherwise I expect the merge is going to be a little challenging.

joperezr · 2018-02-27T23:14:09Z

sounds good, I'll wait for the merge from master and then I'll merge this.

ChadNedzlek · 2018-03-08T22:14:58Z

Is this making progress? 9 days ago we said "later this week", but seems like there hasn't been any activity recently. This is blocking a buildtools uptake which is blocking some infrastructure work.

ericstj · 2018-03-08T23:15:08Z

I think @joperezr mentioned that he was going to undo the buildtools change in release and wait for the changes from master to integrate. @joperezr are you still planning on that?

joperezr · 2018-03-09T21:38:00Z

yup already did that. This work has been now moved to master and merged so I'm closing this PR now.

joperezr requested review from ericstj and weshaggard February 22, 2018 22:00

ericstj reviewed Feb 22, 2018

View reviewed changes

weshaggard reviewed Feb 23, 2018

View reviewed changes

ahsonkhan assigned joperezr Feb 24, 2018

ahsonkhan added the packaging Related to packaging label Feb 24, 2018

ericstj mentioned this pull request Feb 26, 2018

Update CoreClr to preview2-26204-01 (master) #26825

Merged

joperezr added 3 commits February 27, 2018 11:13

Adding to PackageIndex all of the information about our release from …

0cf6b6e

…6.0 and fixing packages that needed placeholders

PR feedback

89d4ad1

Updating Buildtools to get new NuGet mappings and fixing the pkgprojs…

8c885ba

… accordingly

joperezr force-pushed the FixPackageIndex branch from 7ef488d to 8c885ba Compare February 27, 2018 19:19

weshaggard reviewed Feb 27, 2018

View reviewed changes

Fix typo on pkgproj comment

86eb831

corivera mentioned this pull request Feb 28, 2018

Bump System.Data.SqlClient assembly version to 4.4.0. #27011

Merged

karelz added the area-Infrastructure-libraries label Mar 8, 2018

ericstj mentioned this pull request Mar 8, 2018

Update BuildTools, CoreClr, CoreFx, CoreSetup to preview2-02621-01, preview1-26216-04, preview1-26216-02, preview1-26216-03, respectively (release/2.1) #27570

Closed

joperezr closed this Mar 9, 2018

karelz added this to the 2.1.0 milestone Mar 18, 2018

joperezr deleted the FixPackageIndex branch November 2, 2018 20:09

Conversation

joperezr commented Feb 22, 2018

Uh oh!

joshfree commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Feb 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard commented Feb 27, 2018

Uh oh!

joperezr commented Feb 27, 2018

Uh oh!

ChadNedzlek commented Mar 8, 2018

Uh oh!

ericstj commented Mar 8, 2018

Uh oh!

joperezr commented Mar 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants