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

Default empty target group NETCoreApp#13199

Merged
ericstj merged 2 commits intodotnet:masterfrom
ericstj:defaultEmptyTargetGroupNETCoreApp
Nov 1, 2016
Merged

Default empty target group NETCoreApp#13199
ericstj merged 2 commits intodotnet:masterfrom
ericstj:defaultEmptyTargetGroupNETCoreApp

Conversation

@ericstj
Copy link
Copy Markdown
Member

@ericstj ericstj commented Oct 31, 2016

This refactors all builds to make the netcoreapp-latest to be the
default build.

To do this I needed to be specific about the contract project in a few
cases so I made a fix to build tools to permit that and brought over
that fix as part of this commit.

In the future (once we stop building netstandard refs in corefx) we can make this directly enforced with the targets.

/cc @weshaggard @ellismg @chcosta

<ItemGroup Condition="'$(TargetGroup)' == 'net463'">
<ContractProject Include="..\ref\System.Collections.csproj">
<TargetGroup>netstandard1.7</TargetGroup>
</ContractProject>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was confused by this usage, but I see this is new functionality for an older concept - dotnet/buildtools@5da1a3e

What does this do exactly? Does it ensure that there is a project reference to the reference assembly project? Are there scenarios where we actually have to define the OSGroup for these?

<IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netcoreapp1.1'">true</IsPartialFacadeAssembly>
<NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.1</NuGetTargetMoniker>
<IsPartialFacadeAssembly Condition="'$(TargetGroup)' == ''">true</IsPartialFacadeAssembly>
<NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETCoreApp,Version=v1.1</NuGetTargetMoniker>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason not to centrally define the latest values (netcoreapp1.1 and netstandard1.7 plus monikers) and then override them if necessary in the build projects? Might make rolling forward in the future easier.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's what's done for everything but TargetGroup=''. For TargetGroup='' it still means something different for every project. We can't change that yet because we still have a ton of projects that build portable assets this way.

@ericstj
Copy link
Copy Markdown
Member Author

ericstj commented Nov 1, 2016

This is breaking because of another breaking change in buildtools: dotnet/buildtools@2ba3679#diff-c368ca147f7a6d95f33280f334ff9e84R109

/cc @karajas

@weshaggard
Copy link
Copy Markdown
Member

Is this complete because it seems small? I suppose you have only flipped the default for any projects that already have a specific netcoreapp1.1 build. Will the next step be to add that configuration for all the projects?

@ericstj
Copy link
Copy Markdown
Member Author

ericstj commented Nov 1, 2016

Right now this is just making it true that a filter for TargetGroup='' and whatever OSGroup is appropriate will give you all of the NETCoreApp assets needed for all packages.

@ericstj ericstj force-pushed the defaultEmptyTargetGroupNETCoreApp branch from 22669b6 to 9055042 Compare November 1, 2016 17:44
@ericstj
Copy link
Copy Markdown
Member Author

ericstj commented Nov 1, 2016

This is going to fail due to dotnet/buildtools@2ba3679#commitcomment-19651092, putting a fix in buildtools.

@ericstj ericstj force-pushed the defaultEmptyTargetGroupNETCoreApp branch from 9055042 to d4ca152 Compare November 1, 2016 18:53
@ericstj
Copy link
Copy Markdown
Member Author

ericstj commented Nov 1, 2016

@alexperovich the latest failure looks like it is code-analysis related.

CSC : error CS8032: An instance of analyzer Desktop.Analyzers.CSharpDoNotUseInsecureCryptographicAlgorithmsAnalyzer cannot be created from /mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_debug_prtest/Tools/analyzers/Desktop.CSharp.Analyzers.dll : Could not load file or assembly 'Desktop.Analyzers, Version=1.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'. The system cannot find the file specified. [/mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_debug_prtest/src/Microsoft.CSharp/src/Microsoft.CSharp.csproj]

Looks like its happening on all unix builds.

@ericstj
Copy link
Copy Markdown
Member Author

ericstj commented Nov 1, 2016

Seems to be a difference between desktop and core. I ran core CSC on a windows machine and repro'ed the same issue. It looks like Roslyn's AssemblyLoadContext doesn't look next to the analyzer, nor in the list of analyzers when trying to find dependencies. /cc @tmat

@tmat
Copy link
Copy Markdown
Member

tmat commented Nov 1, 2016

@tmeschter @mavasani to comment on the analyzer issue

This refactors all builds to make the netcoreapp-latest to be the
default build.

To do this I needed to be specific about the contract project in a few
cases so I made a fix to build tools to permit that and brought over
that fix as part of this commit.
@ericstj ericstj force-pushed the defaultEmptyTargetGroupNETCoreApp branch from d4ca152 to cdaaefc Compare November 1, 2016 21:26
@ericstj
Copy link
Copy Markdown
Member Author

ericstj commented Nov 1, 2016

Worked around that issue: dotnet/buildtools#1185. Now hopefully will have a clean build 🙏

@tmeschter
Copy link
Copy Markdown

The loader used by core intentionally doesn't look next to an analyzer to find dependencies; it only checks the files in the list of analyzers. It looks like we have a bug in that logic.

On desktop the compiler is a little more forgiving since we just use LoadFrom, and it will look next to loaded assemblies for dependencies. This isn't the desired behavior so much as it is an unavoidable one on desktop.

@tmeschter
Copy link
Copy Markdown

@ericstj Could you detail how you repro'd this on Windows?

@ericstj
Copy link
Copy Markdown
Member Author

ericstj commented Nov 1, 2016

@tmeschter: see dotnet/roslyn#14866. Thanks for opening @alexperovich

@ericstj ericstj merged commit 3bdd0ac into dotnet:master Nov 1, 2016
@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
@ericstj ericstj deleted the defaultEmptyTargetGroupNETCoreApp branch May 9, 2019 04:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants