Only add placeholder pkg file if folder is empty #67647

ViktorHofer · 2022-04-06T13:36:18Z

Placeholder files are added unconditionally by the .NETStandard compat
error packaging infrastructure, even if the
buildTransitive/$(SupportedTFM) package folder isn't empty.

That hinders our libraries to package their own set of buildTransitive
props and targets files for the supported set of tfms.

This change makes sure that placeholder files are added only if no None
or Content items are declared that point to the same package folder.

Adding documentation that explains the .NETStandard compatibility
packaging infrasturcture and how to correctly package hand-authored
msbuild files next to the generated targets files.

ghost · 2022-04-06T13:36:29Z

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #63413

Placeholder files are added unconditionally by the .NETStandard compat
error packaging infrastructure, even if the
buildTransitive/$(SupportedTFM) package folder isn't empty.

That hinders our libraries to package their own set of buildTransitive
props and targets files for the supported set of tfms.

This change makes sure that placeholder files are added only if no None
or Content items are declared that point to the same package folder.

Adding documentation that explains the .NETStandard compatibility
packaging infrasturcture and how to correctly package hand-authored
msbuild files next to the generated targets files.

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	`area-Infrastructure-libraries`
Milestone:	7.0.0

eng/packaging.targets

ViktorHofer · 2022-04-06T15:04:34Z

@eerhardt any objections if I fix #60870 as well to actually test if that works together?

eerhardt · 2022-04-06T15:52:22Z

any objections if I fix #60870 as well to actually test if that works together?

No objections from me. I was going to do it after your change. But if you want to take it here, that's totally fine with me.

ViktorHofer · 2022-04-06T20:07:32Z

@eerhardt the change is ready to be reviewed.

ViktorHofer · 2022-04-06T20:09:44Z

eng/packaging.targets

-         BeforePack must be used. Setting both to ensure that we are always running before other targets. --> 
-    <PackDependsOn>AddNETStandardCompatErrorFileForPackaging;IncludeAnalyzersInPackage;$(PackDependsOn)</PackDependsOn>
-    <BeforePack>AddNETStandardCompatErrorFileForPackaging;IncludeAnalyzersInPackage;$(BeforePack)</BeforePack>
+    <BeforePack>$(BeforePack);IncludeAnalyzersInPackage;AddNETStandardCompatErrorFileForPackaging</BeforePack>


PackDependsOn doesn't work anymore with the sequencing that we depend on. The BeforePack hook is fine as we don't rely on NuGet.Build.Tasks.Pack as an OOB nuget package anymore.

ViktorHofer · 2022-04-06T20:17:05Z

eng/packaging.targets

+      <ItemGroup>
+        <None Include="$(PlaceholderFile)"
+              Pack="true"
+              PackagePath="$(BuildOutputTargetFolder)\$(NetFrameworkMinimum)\"
+              Condition="'$(AddNETFrameworkPlaceholderFileToPackage)' == 'true'" />
+        <None Include="$(PlaceholderFile)"
+              Pack="true"
+              PackagePath="$(BuildOutputTargetFolder)\MonoAndroid10\;
+                           $(BuildOutputTargetFolder)\MonoTouch10\;
+                           $(BuildOutputTargetFolder)\xamarinios10\;
+                           $(BuildOutputTargetFolder)\xamarinmac20\;
+                           $(BuildOutputTargetFolder)\xamarintvos10\;
+                           $(BuildOutputTargetFolder)\xamarinwatchos10\"
+              Condition="'$(AddXamarinPlaceholderFilesToPackage)' == 'true'" />


That's just clean-up to avoid multiple items.

ViktorHofer · 2022-04-06T20:17:58Z

eng/packaging.targets

+  <!-- Include a netstandard compat error if the project targets both .NETStandard and
+       .NETCoreApp. This prohibits users to consume packages on an older .NETCoreApp version
+       than the minimum supported one. -->
+  <ItemGroup>
+    <NETStandardCompatError Include="netcoreapp2.0"
+                            Supported="$(NetCoreAppMinimum)"
+                            Condition="$(TargetFrameworks.Contains('netstandard2.')) and
+                                       ($(TargetFrameworks.Contains('$(NetCoreAppMinimum)')) or $(TargetFrameworks.Contains('$(NetCoreAppCurrent)')))" />
+    <NETStandardCompatError Include="net461"
+                            Supported="$(NetFrameworkMinimum)"
+                            Condition="$(TargetFrameworks.Contains('netstandard2.0')) and
+                                       ($(TargetFrameworks.Contains('$(NetFrameworkMinimum)')) or $(TargetFrameworks.Contains('net47')) or $(TargetFrameworks.Contains('net48')))" />
+  </ItemGroup>


This block was moved up without any changes.

It looks like

'$(DisableNETStandardCompatErrorForNETCoreApp)' != 'true'" />

was removed in the move.

Right, those two items are now always defined but there is a new switch to disable the NETStandard compat errors. (which isn't used anywhere but it's good to have it)

Originally reported in dotnet#63413, placeholder files are added unconditionally by the .NETStandard compat error packaging infrastructure, even if the buildTransitive/$(SupportedTFM) folder isn't empty. That hinders our libraries to package their own set of buildTransitive props and targets files for the supported set of tfms. This change makes sure that placeholder files are added only if no None or Content items are declared that point to the same package folder. Adding documentation that explains the .NETStandard compatibility packaging infrasturcture and how to correctly package hand-authored msbuild files next to the generated targets files.

eng/packaging.targets

src/libraries/System.Resources.Extensions/src/System.Resources.Extensions.csproj

eerhardt

Just a few comments. Nothing major.

am11 · 2022-04-13T13:22:24Z

@ViktorHofer, fyi, codeflow is hitting the following error, probably related to this change (diff: 5707668...f2ce5e8):

error : System.Text.Encodings.Web doesn't support net7.0. Consider updating your TargetFramework to net6.0 or later.

(error message itself seems erroneous since net7.0 > net6.0 🤔)

dotnet/aspnetcore#41166
dotnet/sdk#24837

eerhardt · 2022-04-13T14:10:15Z

Looks like the issue is that the placeholder file is getting added as a folder?

am11 · 2022-04-13T14:59:59Z

Just noticed that net461 will be out of support on April 22, 2022. We may want to drop it from buildTransitive.

eerhardt · 2022-04-13T15:05:31Z

Just noticed that net461 will be out of support on April 22, 2022. We may want to drop it from buildTransitive.

That .targets file is there to give users errors/warnings that net461 is not supported.

<Project InitialTargets="NETStandardCompatError_System_Text_Encodings_Web_net462">
  <Target Name="NETStandardCompatError_System_Text_Encodings_Web_net462"
          Condition="'$(SuppressTfmSupportBuildWarnings)' == ''">
    <Error Text="System.Text.Encodings.Web doesn't support $(TargetFramework). Consider updating your TargetFramework to net462 or later." />
  </Target>
</Project>

ViktorHofer · 2022-04-13T15:33:01Z

Oh thanks... Will submit a fix asap

ViktorHofer added the area-Infrastructure-libraries label Apr 6, 2022

ViktorHofer added this to the 7.0.0 milestone Apr 6, 2022

ViktorHofer requested a review from eerhardt April 6, 2022 13:36

ViktorHofer self-assigned this Apr 6, 2022

eerhardt reviewed Apr 6, 2022

View reviewed changes

eng/packaging.targets Outdated Show resolved Hide resolved

eerhardt reviewed Apr 6, 2022

View reviewed changes

eng/packaging.targets Outdated Show resolved Hide resolved

ViktorHofer force-pushed the PkgMSBuildPropsTargetsNETStandardCompatInfra branch from 09ac364 to 2bfcb48 Compare April 6, 2022 20:02

ViktorHofer commented Apr 6, 2022

View reviewed changes

ViktorHofer force-pushed the PkgMSBuildPropsTargetsNETStandardCompatInfra branch from 2bfcb48 to 4d69282 Compare April 6, 2022 20:16

ViktorHofer commented Apr 6, 2022

View reviewed changes

ViktorHofer force-pushed the PkgMSBuildPropsTargetsNETStandardCompatInfra branch from 4d69282 to 4398a31 Compare April 6, 2022 20:27

eerhardt reviewed Apr 6, 2022

View reviewed changes

eng/packaging.targets Show resolved Hide resolved

eerhardt reviewed Apr 6, 2022

View reviewed changes

src/libraries/System.Resources.Extensions/src/System.Resources.Extensions.csproj Show resolved Hide resolved

eerhardt approved these changes Apr 6, 2022

View reviewed changes

runfoapp bot mentioned this pull request Apr 6, 2022

TLS1.3 networking tests are flaky on Windows 11 #58927

Closed

ViktorHofer added 2 commits April 7, 2022 11:20

Fix NETStandardCompatError empty cases

b62128f

Update packaging.targets

820d377

ViktorHofer merged commit 538934c into dotnet:main Apr 12, 2022

ViktorHofer deleted the PkgMSBuildPropsTargetsNETStandardCompatInfra branch April 12, 2022 20:16

ViktorHofer mentioned this pull request Apr 13, 2022

Fix placeholder files in packages #67960

Merged

ghost locked as resolved and limited conversation to collaborators May 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only add placeholder pkg file if folder is empty #67647

Only add placeholder pkg file if folder is empty #67647

ViktorHofer commented Apr 6, 2022 •

edited

Loading

ghost commented Apr 6, 2022

ViktorHofer commented Apr 6, 2022

eerhardt commented Apr 6, 2022

ViktorHofer commented Apr 6, 2022

ViktorHofer Apr 6, 2022

ViktorHofer Apr 6, 2022

ViktorHofer Apr 6, 2022

eerhardt Apr 6, 2022

ViktorHofer Apr 7, 2022 •

edited

Loading

eerhardt left a comment

am11 commented Apr 13, 2022

eerhardt commented Apr 13, 2022

am11 commented Apr 13, 2022

eerhardt commented Apr 13, 2022

ViktorHofer commented Apr 13, 2022

Only add placeholder pkg file if folder is empty #67647

Only add placeholder pkg file if folder is empty #67647

Conversation

ViktorHofer commented Apr 6, 2022 • edited Loading

ghost commented Apr 6, 2022

ViktorHofer commented Apr 6, 2022

eerhardt commented Apr 6, 2022

ViktorHofer commented Apr 6, 2022

ViktorHofer Apr 6, 2022

Choose a reason for hiding this comment

ViktorHofer Apr 6, 2022

Choose a reason for hiding this comment

ViktorHofer Apr 6, 2022

Choose a reason for hiding this comment

eerhardt Apr 6, 2022

Choose a reason for hiding this comment

ViktorHofer Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

am11 commented Apr 13, 2022

eerhardt commented Apr 13, 2022

am11 commented Apr 13, 2022

eerhardt commented Apr 13, 2022

ViktorHofer commented Apr 13, 2022

ViktorHofer commented Apr 6, 2022 •

edited

Loading

ViktorHofer Apr 7, 2022 •

edited

Loading