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

Update the S.T.E.W configurations to explicitly target a versioned TFM (nc3.0). #42069

Merged
safern merged 6 commits into from
Oct 25, 2019
Merged

Update the S.T.E.W configurations to explicitly target a versioned TFM (nc3.0). #42069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
faadac4
Update the S.T.E.W configurations to explicitly target a versioned TFM
ahsonkhan Oct 23, 2019
8648cf6
Use custom defined constant like S.T.Json - Building_Inbox_Library
ahsonkhan Oct 23, 2019
563725a
Update config.props to use package config and leave netcoreapp as a b…
ahsonkhan Oct 24, 2019
87b013c
Add back netcoreapp specific config to the src csproj
ahsonkhan Oct 24, 2019
1a7a1eb
Revert "Use custom defined constant like S.T.Json - Building_Inbox_Li…
ahsonkhan Oct 24, 2019
4c58ba9
Remove extra new line.
ahsonkhan Oct 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/System.Text.Encodings.Web/src/Configurations.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
<Project DefaultTargets="Build">
<PropertyGroup>
<BuildConfigurations>
netcoreapp;
<PackageConfigurations>
netcoreapp3.0;
Copy link
Copy Markdown
Author

@ahsonkhan ahsonkhan Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Some of the references that S.T.E.W depends on don't have a config for netcoreapp3.0.
System.Diagnostics.Debug
System.Memory
System.Runtime
etc.

Should we also add the versioned TFM configs to all those (along side the version-less ones) or do we need to add package configurations here, instead?

corefx/src/System.Diagnostics.Debug/src/Configurations.props

Lines 4 to 5 in bd18c12

netcoreapp-Unix;
netcoreapp-Windows_NT;

corefx/src/System.Memory/src/Configurations.props

Lines 4 to 5 in bd18c12

netcoreapp-Windows_NT;
netcoreapp-Unix;

That's causing the CI failures:

##[error].dotnet\sdk\3.0.100\Microsoft.Common.CurrentVersion.targets(2106,5): error MSB3245: Could not resolve this reference. Could not locate the assembly "System.Runtime". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors.

Copy link
Copy Markdown
Author

@ahsonkhan ahsonkhan Oct 24, 2019

Choose a reason for hiding this comment

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

build -allconfigurations succeeds but targeting individual frameworks (particularly the default - netcoreapp) fails.

Copy link
Copy Markdown
Member

@safern safern Oct 24, 2019

Choose a reason for hiding this comment

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

This library is inbox as well so you will need a version-less build configuration but you will need to exclude it from the packages configuration so that it targets the latest.

So you do something like:

<PackageConfigurations>
  netcoreapp3.0;
  netstandard2.1;
  netstandard;
</PackageConfigurations>
<BuildConfigurations>
  $(PackageConfigurations);
  netcoreapp;
</BuildConfigurations>

Copy link
Copy Markdown
Author

@ahsonkhan ahsonkhan Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So you do something like

Thanks for the suggestion, @safern. That's precisely what I tried next, following precedence of other similar packages like Threading.Channels:

corefx/src/System.Threading.Channels/src/Configurations.props

Lines 2 to 12 in bd18c12

<PropertyGroup>
<PackageConfigurations>
netstandard1.3;
netstandard;
netcoreapp3.0;
</PackageConfigurations>
<BuildConfigurations>
$(PackageConfigurations);
netcoreapp;
</BuildConfigurations>
</PropertyGroup>

The netcoreapp3.0 package config still causes the failure though. I don't know if its because of contention caused by netstandard2.1 and netcoreapp3.0.

Let me retry that to verify.

Copy link
Copy Markdown
Author

@ahsonkhan ahsonkhan Oct 24, 2019

Choose a reason for hiding this comment

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

Awesome, that worked! I wasn't updating the Configurations property in the csproj locally to include netcoreapp which is why I was seeing the failure earlier.

netstandard2.1;
netstandard;
</PackageConfigurations>
<BuildConfigurations>
$(PackageConfigurations);
netcoreapp;
</BuildConfigurations>
</PropertyGroup>
</Project>
2 changes: 1 addition & 1 deletion src/System.Text.Encodings.Web/src/System.Text.Encodings.Web.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<PropertyGroup>
<RootNamespace>System.Text.Encodings.Web</RootNamespace>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<Configurations>netcoreapp-Debug;netcoreapp-Release;netstandard-Debug;netstandard-Release;netstandard2.1-Debug;netstandard2.1-Release</Configurations>
<Configurations>netcoreapp-Debug;netcoreapp-Release;netcoreapp3.0-Debug;netcoreapp3.0-Release;netstandard-Debug;netstandard-Release;netstandard2.1-Debug;netstandard2.1-Release</Configurations>
</PropertyGroup>
<ItemGroup>
<Compile Include="System\Text\Encodings\Web\DefaultJavaScriptEncoder.cs" />
Expand Down