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

Remove unnecessary placeholders in .nuspec's #18214

Merged
merged 2 commits into from
Oct 10, 2019
Merged

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Oct 3, 2019

In release/3.0, we needed placeholder files for NetStandard1.3 and NetCoreApp2.1 in these 2 nuspecs, but that's not the case in release/3.1 or master. Their existence caused nuget package validation to fail.

@dougbu @Pilchie do you think it's worth taking this for 3.1-preview1? I lean towards waiting for preview2, since it doesn't block the build, and the effect on customers is minimal (an extra empty file in these 2 nupkg's).

@wtgodbe wtgodbe added this to the 3.1.0 milestone Oct 3, 2019
@wtgodbe wtgodbe requested a review from a team October 3, 2019 21:44
@wtgodbe wtgodbe mentioned this pull request Oct 3, 2019
@Pilchie
Copy link
Member

Pilchie commented Oct 3, 2019

Can you explain what changed such that we don't need it anymore?

@wtgodbe
Copy link
Member Author

wtgodbe commented Oct 3, 2019

These 2 packages contain executables & don't place anything in their lib folders, which nuget expects them to do. In 3.1/master, the package has the same TargetFramework as the rest of the repo, and handles placing the placeholder file in a different way: https://github.com/aspnet/EntityFrameworkCore/blob/release/3.1/src/dotnet-ef/dotnet-ef.nuspec#L15

It looks like what happened was we handled this nuget error differently in the 2 branches, and since 3.0 merges up to 3.1/master, and 3.0 has hardcoded netcoreapp2.1 / netstandard1.3 TFMs for these 2 packages (which don't match 3.1/master), we wound up with a redundant workaround for TFMs that didn't need to exist in the packages.

We could just change the TFMs in 3.0 to match 3.1/master, and use the same workaround in all 3 branches. Or given that we're already in a weird-ish state, we could take this PR, then resolve the auto-merge PR in favor of 3.1.

@@ -13,6 +13,5 @@
<file src="$OutputBinary$" target="analyzers\dotnet\cs\" />
<file src="$OutputSymbol$" target="analyzers\dotnet\cs\" />
<file src="$PackageIcon$" target="" />
<file src="$NetStandard13PlaceholderFile$" target="lib\netstandard1.3" />
Copy link
Member

Choose a reason for hiding this comment

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

It's hard to tell in GitHub. Are you deleting the _._ files?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@Pilchie
Copy link
Member

Pilchie commented Oct 3, 2019

Definitely want to hear from the EF team here.

@smitpatel
Copy link
Member

The code being deleted was added by build team only during release/3.0 and now it is being removed.
Questions:

  • Why did EFCore.Analyzers needed placeholder for ns1.3 in 3.0 but does not need for ns2.0 in 3.1. (Brice updated TFM for that). Also EFCore.Analayzer targets different tfm than rest of the repo.
  • dotnet-ef - csproj targeted netcoreapp3.0 and moved to 3.1 now. (intentional? @bricelam). The nuspec defined tfm to be netcoreapp2.1 & related placeholder in 3.0. Now it does not have that code and uses netcoreapp3.1 tfm. What was the reason that we used tfm (for some purpose) as netcoreapp2.1 in 3.0 release but we are using netcoreapp3.1 now.
  • Did anything change in build infra which required placeholder for tfms in past and now it does not.

@wtgodbe
Copy link
Member Author

wtgodbe commented Oct 4, 2019

I believe we started needing these placholders due to a recent Nuget update. I don't know why we decided to change the TFMs in 3.1, but that's what led to the disparity between the 2 workarounds.

Why did EFCore.Analyzers needed placeholder for ns1.3 in 3.0 but does not need for ns2.0 in 3.1. (Brice updated TFM for that).

The following as added in 3.1, which handled the placeholder for Netstandard2.0. The NetStandard1.3 placeholder became redundant at that point.
https://github.com/aspnet/EntityFrameworkCore/pull/18214/files#diff-45710a11efabd62dcc38b8679bdea05cR12

@Pilchie
Copy link
Member

Pilchie commented Oct 4, 2019

I'm going to guess that changing the TFM in 3.1 wasn't the right thing to do :-/

@dougbu
Copy link
Member

dougbu commented Oct 4, 2019

I'm going to guess that changing the TFM in 3.1 wasn't the right thing to do :-/

I seem to remember a conversation in one of the GitHub PRs where the resolution for the tool was to move forward. Of course, that may have been the wrong call and we should definitely do the Right Thing:tm: going forward. (@ajcvickers may have been out when we had that conversation.)

So, @aspnet/efteam does the latest and greatest dotnet ef command need to be runnable against older projects e.g. netcoreapp2.1 ones?

@bricelam
Copy link
Contributor

bricelam commented Oct 4, 2019

Does the latest dotnet ef command need to be runnable against older projects e.g. netcoreapp2.1 ones?

Yes. But that doesn't have anything to do with the target framework of dotnet-ef. We should target the same same framework as the tools that ship as part of the SDK. It's reasonable to say that you need the 3.1 SDK to use dotnet-ef 3.1.

The target frameworks of the ef project determine which target frameworks a project can use. (Should be net461+netcoreapp2.0)

@leecow
Copy link
Member

leecow commented Oct 8, 2019

tactics approved

@wtgodbe
Copy link
Member Author

wtgodbe commented Oct 10, 2019

@bricelam is there work necessary in the dotnet-ef project in 3.1? If so, should it be done in this PR, or separately in another PR for 3.1-preview2?

@bricelam
Copy link
Contributor

Seems fine. Filed #18316 to double-check that everything is working as expected.

@wtgodbe
Copy link
Member Author

wtgodbe commented Oct 10, 2019

Merging since release/3.1 is already the preview2 branch

@wtgodbe wtgodbe merged commit 54a9474 into release/3.1 Oct 10, 2019
@wtgodbe wtgodbe deleted the FixupNuspecs branch October 10, 2019 19:34
@ajcvickers ajcvickers removed this from the 3.1.0 milestone Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants