Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add missed packages to packages.builds for 2.1.5#32203

Merged
wtgodbe merged 7 commits intodotnet:release/2.1from
wtgodbe:PackageBuild215
Sep 11, 2018
Merged

Add missed packages to packages.builds for 2.1.5#32203
wtgodbe merged 7 commits intodotnet:release/2.1from
wtgodbe:PackageBuild215

Conversation

@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Sep 10, 2018

I went through all changes to corefx\src in release/2.1 since 2.1.0, and I believe that these are all of the packages that should have been shipped for servicing, but weren't.

@weshaggard @safern PTAL

CC @danmosemsft

[edit] Here's the list of the original 2.1 fix PR's

There are likely other issues that would not be shipped without this PR. The above is just the first change in the library. Eg., #31680

<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

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

Lets keep these in dir.props/directory.build.props file for consistency and to match your doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I have both locations called out in the doc, but I'm fine with aiming to move them into dir.props just for consistency.

<Import Project="..\dir.props" />
<PropertyGroup>
<AssemblyVersion>4.0.3.0</AssemblyVersion>
<AssemblyVersion>4.0.3.1</AssemblyVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Conflicts with #32111. @bartonjs I know I told you to bump it to 4.0.4.0 in your PR but after further consideration I think we should only bump the revision to avoid needing to keep bumping master as well. So what we have here should be the right version bump.

Copy link
Member

Choose a reason for hiding this comment

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

Presumably we should just close #32111 and take this as the "fix everything" change?

Copy link
Member

Choose a reason for hiding this comment

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

I suspect so.

Copy link
Member Author

Choose a reason for hiding this comment

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

@weshaggard do I also need to update packageIndex.json with the new AssemblyVersions, like https://github.com/dotnet/corefx/pull/32111/files#diff-122916076db7087dbc454352fada61eeR4293? If so I should update the doc as well.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect you might. You will hopefully figure out when the all configuration leg fails.

@danmoseley danmoseley added the Servicing-consider Issue for next servicing release review label Sep 10, 2018
<Import Project="..\dir.props" />
<PropertyGroup>
<AssemblyVersion>4.0.0.0</AssemblyVersion>
<AssemblyVersion>4.0.0.1</AssemblyVersion>
Copy link
Member

Choose a reason for hiding this comment

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

@natemcmaster FYI we are bumping the assembly version of System.IO.Pipelines for 2.1.5 servicing.

Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

LGTM - Please do a local package build to make sure the package versions build and look correct.

@safern
Copy link
Member

safern commented Sep 11, 2018

Will per: #32108 (comment) I added commit 4e4dc12 to fix the System.Drawing.Common assembly version.

@jamshedd jamshedd added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 11, 2018
@jamshedd
Copy link
Member

Approved for 2.1.5.

"InboxOn": {},
"AssemblyVersionInPackageVersion": {
"4.0.0.0": "4.5.0"
"4.0.0.0": "4.5.1",
Copy link
Member

@ViktorHofer ViktorHofer Sep 11, 2018

Choose a reason for hiding this comment

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

The assembly version's package version should be the one in which the assembly with the given version shipped the first time. In this case 4.5.0 and not 4.5.1. Same for others below/above.

Copy link
Member

Choose a reason for hiding this comment

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

No it should be the latest package that shipped stable. We want to ensure that folks are getting the latest version of any changes with the same assembly version.

Copy link
Member

Choose a reason for hiding this comment

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

hmm makes sense but @ericstj told me the opposite in a random PR which I can't find right now.

@weshaggard
Copy link
Member

The tests are failing because we didn't update the assembly version for the il project. We need to also bump the version in the .il file for this particular project

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 11, 2018

@weshaggard updated

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 11, 2018

@ericstj do you recognize the failures here?

Tools\Packaging.targets(1131,5): error : System.Security.Principal.Windows should support API version 4.1.1.1 on Xamarin.WatchOS,Version=v1.0 but D:\j\workspace\windows-TGrou---27e62afc\bin\ref\System.Security.Principal.Windows\4.1.1.0\netstandard\System.Security.Principal.Windows.dll was found to support 4.1.1.0.

It looks like we get the same error for a variety of platforms. Do I need to modify the InboxOn section?

@weshaggard
Copy link
Member

No I don't believe this wouldn't require a modification of InboxOn, The issue is stemming from 7ee51be#diff-c7bf0a95d7488e5968fd7aeb0078fda2 which explicitly pinned the version of the ref which I think is OK. The error is saying the ref is lower version then the implementation which should be valid. I'm not sure why it is complaining, do we need to suppress this check @ericstj?

@ericstj
Copy link
Member

ericstj commented Sep 11, 2018

That's happening because the supported framework is applied to the cross-targeting-csproj reference, which will pull the max of all versions from that ref.

<ProjectReference Include="..\ref\System.Security.Principal.Windows.csproj">
<SupportedFramework>net461;netcoreapp2.0;$(UAPvNextTFM);$(AllXamarinFrameworks)</SupportedFramework>
</ProjectReference>

In this case the max will be the netfx version. Which is why this is failing.

@weshaggard dealt with the same issue here: 2414032#diff-1345536edae72c012e218f0374fa418cR6

In his case he gets the support checks for the other frameworks from harvesting. An alternate to this is to explicitly list SupportedFrameworks as follows:
https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/Documentation/coding-guidelines/package-projects.md#applicability-validation

So the change would be:

<ProjectReference Include="..\ref\System.Security.Principal.Windows.csproj"> 
  <SupportedFramework>net461</SupportedFramework> 
</ProjectReference> 
<SupportedFramework Include="netcoreapp2.0;$(UAPvNextTFM);$(AllXamarinFrameworks)" >
  <Version>4.1.1.0</Version>
</SupportedFramework>

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 11, 2018

Test failure was a Jenkins issue

@dotnet-bot test Windows x86 Release Build

@wtgodbe wtgodbe merged commit 047ad36 into dotnet:release/2.1 Sep 11, 2018
@danmoseley danmoseley modified the milestones: 2.1.x, 2.1.5 Sep 13, 2018
@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-approved Approved for servicing release labels Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants