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.

Sign up for GitHub

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

Jump to bottom

Split analyzers into separate directories #1419

Merged
merged 2 commits into from
Sep 3, 2018
Merged

Split analyzers into separate directories #1419

merged 2 commits into from
Sep 3, 2018

Conversation

thomaslevesque
Copy link
Member

This gets rid of two warnings that had been annoying me for a while:

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets(818,5): warning MSB3539: The value of the property "BaseIntermediateOutputPath" was modified after it was used by MSBuild which can lead to unexpected build results. Tools such as NuGet will write outputs to the path specified by the "MSBuildProjectExtensionsPath" instead. To set this property, you must do so before Microsoft.Common.props is imported, for example by using Directory.Build.props. For more information, please visit https://go.microsoft.com/fwlink/?linkid=869650 [C:\projects\fakeiteasy\src\FakeItEasy.Analyzer\FakeItEasy.Analyzer.VisualBasic.csproj]

And it's cleaner anyway. Having two projects in the same directory forced us to jump through hoops, like specifying a different intermediate output path (objcs/objvb instead of obj), and explicitly excluding files from the other project's intermediate output path.

thomaslevesque
thomaslevesque commented Sep 1, 2018
View reviewed changes
src/FakeItEasy.Analyzer.CSharp/FakeItEasy.Analyzer.CSharp.csproj Outdated
<PropertyGroup>
<TargetFramework>netstandard1.1</TargetFramework>
<AssemblyName>FakeItEasy.Analyzer.CSharp</AssemblyName>
Copy link
Member Author

Choose a reason for hiding this comment

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

Implicit

src/FakeItEasy.Analyzer.CSharp/FakeItEasy.Analyzer.CSharp.csproj
<RootNamespace>FakeItEasy.Analyzer</RootNamespace>
<AppDesignerFolder>Properties</AppDesignerFolder>
<GenerateAssemblyInfo>false</GenerateAssemblyInfo>
<FileAlignment>512</FileAlignment>
<SignAssembly>true</SignAssembly>
<DefineConstants>$(DefineConstants);CSHARP</DefineConstants>
<PackageTargetFallback>portable45-net45+win8</PackageTargetFallback>
<BaseIntermediateOutputPath>objcs\</BaseIntermediateOutputPath>
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer necessary

src/FakeItEasy.Analyzer.CSharp/FakeItEasy.Analyzer.CSharp.csproj
<Compile Remove="objvb\**" />
<EmbeddedResource Remove="objvb\**" />
<None Remove="objvb\**" />
</ItemGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer necessary

src/FakeItEasy.Analyzer.CSharp/FakeItEasy.Analyzer.CSharp.csproj

<PropertyGroup Condition="'$(Configuration)' == 'Release'">
<DebugType>pdbonly</DebugType>
</PropertyGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to ../FakeItEasy.Analyzer.Shared/Directory.Build.props

src/FakeItEasy.Analyzer.Shared/DiagnosticDefinitions.cs
private const string ResourceBaseName = "FakeItEasy.Analyzer.CSharp.Resources";
#elif VISUAL_BASIC
private const string ResourceBaseName = "FakeItEasy.Analyzer.VisualBasic.Resources";
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

The CSharp/VisualBasic subfolders are no longer necessary

src/FakeItEasy.Analyzer.Shared/Directory.Build.props Outdated
<Content Include="$(MSBuildThisFileDirectory)tools/*.ps1" Pack="true" PackagePath="tools/%(Filename)%(Extension)" />
</ItemGroup>

</Project>
Copy link
Member Author

Choose a reason for hiding this comment

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

While I was at it, I extracted some common bits to this file.
The file name might be misleading, though, since it's not a "real" Directory.Build.props (in the sense that it's not implicitly imported). Maybe we should rename it to something else, e.g. FakeItEasy.Analyzers.Shared.proj.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy as is, or if you'd prefer another name, that's fine too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to try something. Currently it's difficult to add new files to the projects, because there's no place to do it in the solution explorer. Introducing a shared project might make things easier (I'm not sure it works with the .NET Core SDK though, I need to try it)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, it works with a shared project, but it's not perfect. Shared projects don't have all the nice features of SDK-style projects, so the files are listed explicitly in the .projitems file (we can manually add glob patterns, but VS doesn't really support it so new files are added explicitly, which causes them to be included twice in the build). But at least we can now add files to the right place from Visual Studio.

Let me know what you think.

blairconrad
blairconrad previously approved these changes Sep 2, 2018
View reviewed changes
Copy link
Member

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

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

Thanks, @thomaslevesque. That warning was annoying, and I've never loved the objcs/objvb dirs. You've some conflicts to resolve, but other than that, and whatever you prefer for the Directory.Build.props file name, I'm content!

@thomaslevesque thomaslevesque changed the title Split analyzers into separate directories [WIP] Split analyzers into separate directories Sep 2, 2018
@thomaslevesque thomaslevesque mentioned this pull request Sep 2, 2018
Closed
@blairconrad
Copy link
Member

@thomaslevesque, sorry, I'm a little behind.

First, I don't really have any strong opinions about the organization of the projects. If you're happy with things, and think I worry too much, we can merge it up. But…

You've made some compromises around the Directory.Bulid.props and the shared project (with the .projitems file) and so on. I think you're unhappy with them.
Have you considered extracting a regular project for the common functionality? Maybe there's a technical reason not to, such as analyzers hating being multi-assembly, or other awkwardness.
I could investigate, but am lazy and barely keeping up with what's been going on here (and in related repos).

@thomaslevesque
Copy link
Member Author

Have you considered extracting a regular project for the common functionality?

Well, the problem is that most of the code is apparently identical, but with types that are actually in different namespaces (*.CSharp vs *.VisualBasic). So we can't just compile a "common" project and reference it from the C# and VB analyzers, because almost nothing is actually common. It's all based on #ifs.

I'm not super happy with the shared project solution, mostly because it's an old-style project where source files are explicitly listed, but I can live with it. I opened dotnet/sdk#2511, hopefully a future version of the SDK will make things a bit better.

Anyway, I'm content to merge as is if you have no remarks. Even if it's not perfect, it's going to solve a few issues (e.g. being able to upgrade to a newer SDK, maybe used dotnet build rather than MSBuild...)

@thomaslevesque thomaslevesque changed the title [WIP] Split analyzers into separate directories Split analyzers into separate directories Sep 3, 2018
@thomaslevesque
Copy link
Member Author

rebased, and removed [WIP]

blairconrad
blairconrad approved these changes Sep 3, 2018
View reviewed changes
Copy link
Member

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

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

Thanks, @thomaslevesque. I agree that it's worth merging this on its own merits, and unblocking other issues makes it even more worthwhile. If we decide to adjust later, so be it!

@blairconrad blairconrad merged commit 7b56464 into FakeItEasy:develop Sep 3, 2018
@blairconrad blairconrad added this to the vNext milestone Sep 3, 2018
@thomaslevesque thomaslevesque deleted the split-analyzers branch September 4, 2018 07:42
@thomaslevesque
Copy link
Member Author

Thanks @blairconrad!

@blairconrad
Copy link
Member

This change has been released as part of FakeItEasy 4.8.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blairconrad blairconrad blairconrad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.8.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@thomaslevesque @blairconrad