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

Add Microsoft.AspNetCore.BuildTools.ApiCheck to builds #205

Closed
wants to merge 15 commits into from

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Mar 22, 2017

Add Microsoft.AspNetCore.BuildTools.ApiCheck.Task project

Fall back to RuntimeTargets when RuntimeAssemblies yields no assemblies

Improve project-to-project reference and native assembly handling

  • avoid FileNotFoundException in AssemblyLoadContext.GetAssemblyName() due to \placeholder\ paths
  • catch and ignore BadImageFormatException in AssemblyLoadContext.GetAssemblyName()
  • avoid InvalidOperationException in PackageGraph.RuntimeIsCompatible() due to missing .sha512 file

Change ApiCheck.exe's options and output

  • add api-check --compact-output option
  • generally make tool's output more useful to the task
    • catch and log FileNotFoundException; output types and stack traces of caught Exceptions
  • remove --framework option; unused in .NET Framework already
  • change ApiCheck.exe option from --ApiListing to --api-listing for consistency w/ other options

nits:

  • remove PackageGraph.RuntimeIsCompatible() candidateRuntime parameter
    • not needed and confusingly-named; passed value included in compatibleRuntimes
  • update solution to use current project type GUID
  • ignore global.json and launchSettings.json
  • make some Linq a bit more readable
  • whitespace and using cleanup
  • move all property groups above item groups in Microsoft.AspNetMicrosoft.AspNetCore.BuildTools.ApiCheck.csproj
  • make a couple of constructors private; used only in public static methods
  • ignore case a bit more

notes:

  • don't use UseCommandProcessor==true; ToolTask writes out .cmd file with a BOM that cmd.exe hates

@dnfclas
Copy link

dnfclas commented Mar 22, 2017

@dougbu,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@@ -15,34 +15,36 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "test", "test", "{60A938B2-D95A-403C-AA7A-3683AD64DFA0}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "NuGetPackageVerifier", "src\NuGetPackageVerifier\NuGetPackageVerifier.csproj", "{657AFF5E-164E-493D-8501-8026B7C20808}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "NuGetPackageVerifier", "src\NuGetPackageVerifier\NuGetPackageVerifier.csproj", "{657AFF5E-164E-493D-8501-8026B7C20808}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reverse this? VS probably changed it, but the FAE04EC0 guid is correct. This VS bug is being tracked here: dotnet/project-system#1821

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈 Not a fan of fighting Visual Studio. We can revert this part of the change when dotnet/project-system#1821 is fixed or wait for a future VS update to change the solution for us.

<Import Project="..\..\build\common.props" />

<PropertyGroup>
<TargetFramework>netstandard1.6</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

This task assembly probably needs to cross-compile for net46. Visual Studio runs MSBuild.exe which uses .NET Framework. The netstandard1.6 won't load.

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈 This is intentional. Our engineering team decided not to run the tool within Visual Studio. We may change that after #201 is fixed.

/// <inheritdoc />
protected override string GenerateCommandLineCommands()
{
var arguments = string.Empty;
Copy link
Contributor

Choose a reason for hiding this comment

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

I just added API for this. Can you use it here? See ArgumentEscaper.

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈 Not the Right Thing:tm: for this task. ArgumentEscaper doesn't handle anything but spaces (not all special bash and cmd characters) and this will run in /every/ Universe build. In addition, MSBuild properties like $(TargetPath) are already in reasonable shape and folder names containing double quotes are incredibly unlikely.

return Path.GetFullPath(Path.Combine(taskAssemblyFolder, "..", "net452", ToolExe));
}

var dotnetPath = new FileInfo(AppContext.GetData("FX_DEPS_FILE") as string) // .\Microsoft.NETCore.App.deps.json
Copy link
Contributor

Choose a reason for hiding this comment

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

I just added API for this. Can you use it here? See DotNetMuxer.

#if NETCOREAPP1_1
var lockFile = new LockFileFormat().Read(assetsJson);
var graph = PackageGraph.Create(lockFile, framework);
var graph = PackageGraph.Create(lockFile, "netcoreapp1.1");
Copy link
Contributor

Choose a reason for hiding this comment

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

What about .NET Core 2.0? This adds a problem we're trying to undo in other build tools. See conversation on #121

Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed offline, moving to netcoreapp2.0 hit some roadblocks. Would likely need to build this tool against a specific shared library version to ensure consuming projects run it in something newer. For now, leaving this as-is because it's earlier than what KoreBuild installs.

On the other hand, I have preserved the dependency version cleanup I did to get rid of package downgrades after moving every .exe to netcoreapp2.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me clarify: I didn't mean we upgrade the tool to run on netcoreapp2.0. Rather I was asking if we can allows this tool to parse the graph for multiple versions of netcoreapp1.0.

More context: some other tools hardcode netcoreapp1.0 which led to #121. Hardcoding netcoreapp1.1 is repeating the same mistake. A fix for #121 will likely revert the removal of framework if you commit this as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the context. I'll restore the --framework command-line option.

<Project>
<!-- Set nothing if handled in project file or already done in outer build. -->
<PropertyGroup>
<EnableApiCheck Condition=" '$(EnableApiCheck)' == '' AND '$(MSBuildRuntimeType)' == 'Core' ">true</EnableApiCheck>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this only enabled on dotnet.exe builds? Don't we want to see build errors in VS?

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, we don't want this in developer's faces when they are just hacking things together.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree. We want developers aware of breaking changes. Many times their unintentionally and we don't catch them until the tool actually runs. We have developers who only build + test in VS.

But I'll let others weigh in to settle this. Should API check cause building in VS to fail? Or should it only fail on build.cmd? cc @Eilon @muratg

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈 We've already had this conversation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't aware of a design discussion about this. Please re-consider. Making VS builds and build.cmd as similar as possible is a highly desirable goal.

If the only concern is that api check errors might impede a developer's inner loop, there are better solutions. Two come to mind: (1) make api check failures a warning in Debug but an error in Release. (2) detect "BuildingInsideVisualStudio" and demote errors to being non-fatal.

Copy link
Member

Choose a reason for hiding this comment

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

It was discussed w/ the engineering team that we don't want this to run when building from VS.

<ExcludePublicInternalTypes_InApiCheck Condition=" '$(ExcludePublicInternalTypes_InApiCheck)' == '' ">true</ExcludePublicInternalTypes_InApiCheck>

<!-- Hook API checks into Pack run, prior to creating the .nupkg but after build (if any). -->
<GenerateNuspecDependsOn Condition=" ! $(GenerateNuspecDependsOn.Contains('ApiCheck')) ">$(GenerateNuspecDependsOn);ApiCheck</GenerateNuspecDependsOn>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should append to BuildDependsOn. API Check failures should cause the /t:Build target to fail.

By thew way, MSBuild will de-dup the property so you don't need the .Contains condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll remove the Condition.

Don't understand your point about $(BuildDependsOn) versus $(GenerateNuspecDependsOn). Failures in this target cause command-line builds to fail. Also, the intent is to check the APIs of assemblies we ship (aka pack), not everything we build.

Copy link
Contributor

Choose a reason for hiding this comment

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

Producing an and API check are orthogonal concerns. GenerateNuspecDependsOn is not guaranteed to run as a part of producing an binary.

<Import Project="$(MSBuildThisFileDirectory)Common.props"/>

<PropertyGroup>
<_ApiCheckTaskAssembly>$(MSBuildThisFileDirectory)..\tools\netstandard1.6\Microsoft.AspNetCore.BuildTools.ApiCheck.Task.dll</_ApiCheckTaskAssembly>
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to check for MSBuildRuntimeType and use a differenet assembly if MSBuildRuntimeType != Core. See this example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Intentionally not part of non-Core MSBuild. But, if I left out the condition, I'll add it.

for use outside of Microsoft.
-->
<Project>
<Target Name="ApiCheck" Condition=" '$(EnableApiCheck)' == 'true' AND $(IsPackable) == 'true' ">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does IsPackable need to be enabled for ApiCheck to run?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we don't check code we don't ship.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pack and api check are separate and shouldn't be intermingled. It's possible to produce a binary can end up in a nuget package even when IsPackable is false. EnableApiCheck should be sufficient to disable the target.

The target can use existence of baseline.json files to avoid running.
The target can use the IsTestProject property to suppress the warning on test projects.

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 agreed plan was to do the API Check as part of the Pack target.

@@ -0,0 +1,30 @@
<!--
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks one of our design patterns for tools and targets. To keep our tools composible and to separate concerns, tools packages only deliver the tool. Integration into build cycle is specified by KoreBuild and/or Internal.AspNetCore.Sdk.

For example, the NuGetPackageVerifier package contains the tool, but the target to light it up actually is defined in KoreBuild.
Likewise, the GetGitCommitInfo task is defined and delivered by Internal.AspNetCore.BuildTools.Tasks, but the target that ties it into MSBuild compile step is defined in Internal.AspNetCore.Sdk.

This fix should be simple. Lift the ApiCheck target into Internal.AspNetCore.Sdk instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is that odd distance between tasks and where they are used a "design pattern"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, mainly because consuming the Api Check package directly may be useful in some cases e.g. just to run the tool from a command line. Don't want to affect the consuming project then.

@dougbu
Copy link
Member Author

dougbu commented Mar 24, 2017

🆙📅 with numerous changes. Have a look at recent commit titles to get some sense of what's changed.

This should be ready to go. Running another local Universe build to confirm. But, that's been hanging while cloning repos (before these tools are used) for some reason…

@dougbu
Copy link
Member Author

dougbu commented Mar 24, 2017

… hanging was a temporary issue with my machine. This PR is working.

Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

Please revert the dependency version changes made to anything except ApiCheck.csproj. This needs to done more carefully as this change will break patch builds that will not have access to 1.2.0-* packages. Dependency version changes that break backward compatibility should result in a change to the tool package version number. For instance, this bumps netcoreapp1.0 tools to 4.3.0 CoreFx packages, which was meant for use with netcoreapp1.1.

Please limit the scope your changes those required for making ApiCheck work. We'll take care of version bumps in #200.

Side note: M.Ex.CommandLineUtils doesn't exist anymore. It's now a .Sources only package cref dotnet/extensions#205

@@ -18,7 +18,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="NETStandard.Library" Version="1.6.0" PrivateAssets="All" />
<PackageReference Include="NETStandard.Library" Version="$(NetStandardImplicitPackageVersion)" PrivateAssets="All" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the version bumps. These versions were picked specifically to align with what is in MSBuild for .NET Core 1.0. A dependency version bump in tools packages should result in a tools version bump. This may break patch builds. We'll take care of the upgrade in #200

#if NETCOREAPP1_1
var lockFile = new LockFileFormat().Read(assetsJson);
var graph = PackageGraph.Create(lockFile, framework);
var graph = PackageGraph.Create(lockFile, "netcoreapp1.1");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me clarify: I didn't mean we upgrade the tool to run on netcoreapp2.0. Rather I was asking if we can allows this tool to parse the graph for multiple versions of netcoreapp1.0.

More context: some other tools hardcode netcoreapp1.0 which led to #121. Hardcoding netcoreapp1.1 is repeating the same mistake. A fix for #121 will likely revert the removal of framework if you commit this as is.

</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' != 'netstandard1.0' ">
<PackageReference Include="Microsoft.Extensions.CommandLineUtils" Version="$(AspNetCoreVersion)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This package isn't being produced anymore. Use Microsoft.Extensions.CommandLineUtils.Sources.

<ItemGroup>
<PackageReference Include="Microsoft.DotNet.PlatformAbstractions" Version="$(AspNetCoreVersion)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a different version variable for this one? We don't produce this package and it may not be the same as AspNetCoreVersion. For example, its latest version is 2.0.0-*, not 1.2.0-*

<PackageReference Include="Microsoft.DotNet.ProjectModel" Version="1.0.0-rc3-1-003177" />
<PackageReference Include="Microsoft.DotNet.Cli.Utils" Version="$(DotNetCliUtilsVersion)" />
<PackageReference Include="Microsoft.DotNet.ProjectModel" Version="$(DotNetProjectModelVersion)" />
<PackageReference Include="Microsoft.Extensions.CommandLineUtils" Version="$(AspNetCoreVersion)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

.Sources

<PropertyGroup>
<VersionPrefix>1.0.1</VersionPrefix>
<TargetFrameworks>netcoreapp1.1;net452</TargetFrameworks>
<VersionPrefix>1.0.2</VersionPrefix>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for incrementing the version. This makes it safer to bump dependency versions.

@natemcmaster
Copy link
Contributor

Separate to the changes requested above, I wanted to re-iterate my concern about picking the concept of ApiCheck and producing a NuGet package. These are orthogonal concerns. While we currently only API Check when we also pack with Microsoft.NET.Sdk, it may not always be the way we want to use it. E.g. in current form, it would not work on https://github.com/aspnet/AspNetKatana or https://github.com/aspnet/EntityFramework6.

There are 2 simple changes I'm proposing to decouple API Check from pack targets:

  1. Chain off of Build instead of GenerateNuSpec
  2. Use Condition="Exists('baseline.json')" to determine if a API check should run, not IsPackable=true.

@dougbu
Copy link
Member Author

dougbu commented Mar 24, 2017

Side note: M.Ex.CommandLineUtils doesn't exist anymore. It's now a .Sources only package cref dotnet/extensions#205

That's unexpected. The .Sources package won't work for at least one project. Don't remember which one but we have tested APIs that expose CommandLineUtils types.

@dougbu
Copy link
Member Author

dougbu commented Mar 24, 2017

These are orthogonal concerns.

I agree the concerns are orthogonal. However this is where we decided to hook the task in by default.

in current form, it would not work on https://github.com/aspnet/AspNetKatana or https://github.com/aspnet/EntityFramework6.

All unusual repos will be able to hook the task in differently as they wish. The current approach works fine throughout Universe.

not IsPackable=true

As it turns out, this condition is redundant. I'll remove it.

@natemcmaster
Copy link
Contributor

where we decided

Was that decision made along with the information and concerns I have expressed? We are not bound to bad decisions.

@dougbu
Copy link
Member Author

dougbu commented Mar 24, 2017

this bumps netcoreapp1.0 tools to 4.3.0 CoreFx packages

I agree with this part of your concern. The rest isn't valid because we have no patch branches using MSBuild. So, I'll make sure we're building for netcoreapp1.1 everywhere.

@natemcmaster
Copy link
Contributor

The rest isn't valid because we have no patch branches using MSBuild.

Talked in person with @dougbu. I'm assuming this comment was after that because in person we discussed that we do have patch branches on MSbuild. aspnet/DotNetTools, aspnet/EntityFramework.Tools and aspnet/Scaffolding.

@natemcmaster
Copy link
Contributor

I'm dropping my requests to chain off build not pack, and to run API check in VS. I may disagree with the design, but I'm not part of the decision process so @dougbu is correct here in following the design agreed upon by the build engineering team.

- aspnet/KoreBuild#143

Add Microsoft.AspNetCore.BuildTools.ApiCheck.Task project
  - include task assembly in the Microsoft.AspNetMicrosoft.AspNetCore.BuildTools.ApiCheck package
  - add *.props and *.targets files to hook task into `Pack` target; runs just before .nupkg is created
- reference Microsoft.AspNetMicrosoft.AspNetCore.BuildTools.ApiCheck project from the Sdk project
  - requires lots of hacks; dotnet/sdk#939 workarounds fail in this scenario

Fall back to `RuntimeTargets` when `RuntimeAssemblies` yields no assemblies

Improve project-to-project reference and native assembly handling
- avoid `FileNotFoundException` in `AssemblyLoadContext.GetAssemblyName()` due to `\placeholder\` paths
- catch and ignore `BadImageFormatException` in `AssemblyLoadContext.GetAssemblyName()`
- avoid `InvalidOperationException` in `PackageGraph.RuntimeIsCompatible()` due to missing .sha512 file

Change ApiCheck.exe's options and output
- add `api-check --compact-output` option
- generally make tool's output more useful to the task
  - catch and log `FileNotFoundException`; output types and stack traces of caught `Exception`s
- remove `--framework` option; unused in .NET Framework already
- change ApiCheck.exe option from `--ApiListing` to `--api-listing` for consistency w/ other options

nits:
- remove `PackageGraph.RuntimeIsCompatible()` `candidateRuntime` parameter
  - not needed and confusingly-named; passed value included in `compatibleRuntimes`
- update solution to use current project type GUID
- ignore global.json and launchSettings.json
- make some Linq a bit more readable
- whitespace and `using` cleanup
- move all property groups above item groups in Microsoft.AspNetMicrosoft.AspNetCore.BuildTools.ApiCheck.csproj
- make a couple of constructors `private`; used only in `public static` methods
- ignore case a bit more

notes:
- don't use `UseCommandProcessor==true`; `ToolTask` writes out .cmd file with a BOM that cmd.exe hates
- everything should now be using `NETStandard.Library` v1.6.1, CoreFx packages v4.3.0, etc.
- remove duplicate `<PropertyGroup>` from `NugetReferenceResolver` project
- add two dependencies to `BuildTools.Tasks.Tests` project to avoid package downgrade warnings
- VersionTool project cannot do this because it has `public` properties of these `internal` types
  - just sorted the dependencies
- will file a separate issue to make those properties `internal` if possible; probably low-pri
- don't change default: `$(NetStandardImplicitPackageVersion)` -> `$(NetStandardPackageVersion)`
- don't use `$(IsPackable)` in a condition; redundant and may block direct use of the `ApiCheck` target
@dougbu
Copy link
Member Author

dougbu commented Mar 24, 2017

🆙📅 Rebased on dev and added 4 5 new commits. I restored ApiCheck.exe's --framework option, used that in ApiCheckTask, reverted many dependency version changes, remove an $(IsPackable) check, and used the Microsoft.Extensions.CommandLineUtils.Sources package where possible.

@natemcmaster
Copy link
Contributor

Nice work :)

@@ -1,9 +1,12 @@
<Project>
<PropertyGroup>
<AspNetCoreVersion>1.2.0-*</AspNetCoreVersion>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we reference the versions on NuGet.org? It's kinda weird to reference ci-dev packages from this repo since it runs independent of Universe and needs to exist beyond the lifetime of these packages.

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'll see how much this is used now. If it's just for the CommandLineUtils, I'll leave it as-is. Due to patch releases, might have to use too many versions for NuGet use to be interesting and controllable from this file.

BTW I didn't change this property or add a source to the NuGet.config file 😺

Importance="normal"
Text="Checking for breaking changes in $(MSBuildProjectName), $(TargetFramework)" />
<ApiCheckTask Condition=" '$(_ApiListingFile)' != '' "
ApiListingPath="$(_ApiListingFile)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use the same indentation as C#? Maintaining these indentations over time is just a pain.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is how VS indents XML by default. But, I agree it won't be easy to maintain this in Notepad and VS Code.

I am not changing existing indentation to match. There's a random mix in this repo.

@dougbu
Copy link
Member Author

dougbu commented Mar 25, 2017

@dougbu dougbu closed this Mar 25, 2017
@dougbu dougbu deleted the dougbu/check-api.143 branch March 25, 2017 05:00
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.

None yet

5 participants