Align with other ASP.NET Core projects #232

dougbu · 2018-01-20T04:12:59Z

part of Move ASP.NET WebHooks code to new aspnet/AspNetWebHooks repo #228
use regular LICENSE.txt and Key.snk
use BuildTools
- start with aspnet/BuildTools/tree/dev/scripts/bootstrapper files
- copy other files and pieces of files from aspnet/Mvc repo
- remove properties that BuildTools sets from root Directory.Build.props
use Internal.AspNetCore.Sdk
update version to 1.0.0-preview1
work around Change SlackCommand.ParseParameters(...) to not return a ValueTuple #231 (aka .NET Standard issues on .NET Framework 4.7.1 dotnet/standard#567)

nits:

remove support for <Configuration>CodeAnalysis</Configuration>
centralize references in test projects
remove a few files I missed in Remove ASP.NET WebHooks code and infrastructure #230

Aligning dependency versions (using dependencies.props) will come soon.

- part of #228 - use regular LICENSE.txt and Key.snk - use BuildTools - start with aspnet/BuildTools/tree/dev/scripts/bootstrapper files - copy other files and pieces of files from aspnet/Mvc repo - remove properties that BuildTools sets from root Directory.Build.props - use Internal.AspNetCore.Sdk - update version to 1.0.0-preview1 - work around #231 (aka dotnet/standard#567) nits: - remove support for `<Configuration>CodeAnalysis</Configuration>` - centralize references in test projects - remove a few files I missed in #230 Aligning dependency versions (using dependencies.props) will come soon.

dougbu · 2018-01-20T05:49:35Z

@danroth27 / @Eilon the Readme.md file seems a bit sparse after removing the no-longer-relevant documentation links. Do we have a standard paragraph or two describing ASP.NET Core WebHooks?

- should be working soon-ish

- get builds there working

- build.sh file needs to be executable - not ready for .NET Core App 2.1 with current dependencies

dougbu · 2018-01-20T06:56:48Z

🆙📅 to clean CI builds

dougbu · 2018-01-20T07:18:06Z

build/repo.props

@@ -0,0 +1,14 @@
+<Project>
+  <PropertyGroup>
+    <DisablePackageReferenceRestrictions>true</DisablePackageReferenceRestrictions>


This will go away when I move to ASP.NET Core 2.1 dependencies and add a dependencies.props file.

dougbu · 2018-01-20T07:20:38Z

test/Directory.Build.props

  <PropertyGroup>
-    <_ImportParentFile>false</_ImportParentFile>
-    <_ImportParentFile Condition="$(MSBuildProjectName.StartsWith( 'Microsoft.AspNetCore.' ))">true</_ImportParentFile>
+    <DeveloperBuildTestTfms>netcoreapp2.0</DeveloperBuildTestTfms>


Will change this to netcoreapp2.1 and add netcoreapp2.1 two lines down when I move to ASP.NET Core 2.1 dependencies.

dougbu · 2018-01-20T07:21:41Z

.appveyor.yml

@@ -6,15 +6,17 @@ branches:
    - /^(.*\/)?ci-.*$/
    - /^rel\/.*/
 configuration:
-  - CodeAnalysis
+  - Debug
  - Release


Thoughts? I'm not sure we bother building more than one $(Configuration) elsewhere.

- TFMs weren't lining up

dougbu · 2018-01-20T20:47:48Z

test/Directory.Build.props

  <PropertyGroup>
-    <_ImportParentFile>false</_ImportParentFile>
-    <_ImportParentFile Condition="$(MSBuildProjectName.StartsWith( 'Microsoft.AspNetCore.' ))">true</_ImportParentFile>
+    <DeveloperBuildTestTfms Condition=" '$(DeveloperBuildTestTfms)' == '' ">netcoreapp2.1</DeveloperBuildTestTfms>


FYI this Condition is not in the Mvc version of this file. I added it to allow developers to regularly test two TFMs or otherwise override the $(DeveloperBuild) default.

Does WebHooks have a particular need for that functionality? If not what's the particular use case you see for this?

I don't need to test multiple Core TFMs but testing .NET Framework has caught a couple of problems. My AspNetCoreSettings.props file looks like

<Project> <PropertyGroup> <DeveloperBuild>true</DeveloperBuild> <DeveloperBuildTestTfms>netcoreapp2.1</DeveloperBuildTestTfms> <DeveloperBuildTestTfms Condition=" '$(OS)' == 'Windows_NT' ">$(DeveloperBuildTestTfms);net461</DeveloperBuildTestTfms> </PropertyGroup> </Project>

Without this Condition, my $(DeveloperBuildTestTfms) settings would be ignored. What is the problem with supporting this override?

That said, I could do something more like how we handle $(DotNetRestoreSources) in sources.repo i.e consume a new property that is not defined by default e.g.

<DeveloperBuildTestTfms>$(AdditonalDeveloperBuildTestTargets);netcoreapp2.1</DeveloperBuildTestTfms>

ryanbrandenburg

Mostly questions and clarifications. My big concern is that we not merge this with DisablePackageReferenceRestrictions turned on.

ryanbrandenburg · 2018-01-22T17:23:35Z

Directory.Build.props


  <Import Project="version.props" />
+  <Import Project="build\sources.props" />


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

Discussed offline. Will do this before adding this repo to Universe.

ryanbrandenburg · 2018-01-22T17:46:04Z

Directory.Build.props

    <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
-    <MSBuildAllProjects>$(MSBuildAllProjects);$(RepositoryRoot)version.props</MSBuildAllProjects>
+    <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileDirectory)version.props</MSBuildAllProjects>


Use https://github.com/aspnet/Mvc/blob/dev/Directory.Build.props as an example, we don't do this there, where did you get the idea to do this here?

Depending on a file that can change everything about the build without invalidating previous builds is a bad practice. IOW the MVC version of this file looks like it's missing something important.

That's fine, let's just move it inside version.props.

ryanbrandenburg · 2018-01-22T17:47:28Z

Directory.Build.targets

-    </PackageOutputPath>
+  <!-- Delayed because $(TargetFramework) may not be set when Directory.Build.props is imported. -->
+  <PropertyGroup>
+    <RuntimeFrameworkVersion Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">2.0.0</RuntimeFrameworkVersion>


If you're aligning with other repos these two should be variables.

Coming with dependencies.props. Will add that before adding this repo to Universe.

ryanbrandenburg · 2018-01-22T17:48:40Z

Directory.Build.targets

  </PropertyGroup>

-  <!-- Default items are added after Directory.Build.props runs, causing invalid duplicate entries. -->
-  <ItemGroup Condition="$(_ImportParentFile) AND Exists( '$(MSBuildProjectDirectory)\Properties\Resources.resx' )">
+  <!-- Delayed because default items are added after Directory.Build.props is imported, causing invalid duplicate entries. -->


As someone who's never touched WebHooks this comment doesn't really mean much to me, could you clarify or give more detail?

This isn't really about WebHooks in particular except that WebHooks doesn't add or link resources in individual project files. IOW the <ItemGroup> exists just to remove repeated junk from the projects.

Expanding the comment: The <ItemGroup> isn't in Directory.Build.props because that runs too early. What that means is a version in Directory.Build.props would have to add the <Compile> and <EmbeddedResource> items. Then the default additions would run and complain because something already exists. This way, the items are just updated.

That makes sense now, lets just say "This is in .targets instead of .props because..."

ryanbrandenburg · 2018-01-22T17:50:56Z

README.md

- [Stripe](/samples/StripeReceiver)
- [VSTS](/samples/VstsReceiver)
- [Zendesk](/samples/ZendeskReceiver)
-
 #### ASP.NET Core


Can we get a more descriptive title or add a description here? I'm unclear on what these are links to.

Not sure what's needed, please clarify. This is a sub-section under the Samples heading.

Let's merge this sub-section into its parent.

ryanbrandenburg · 2018-01-22T17:52:57Z

build.cmd

-endlocal
-exit /B 0
+@ECHO OFF
+PowerShell -NoProfile -NoLogo -ExecutionPolicy unrestricted -Command "[System.Threading.Thread]::CurrentThread.CurrentCulture = ''; [System.Threading.Thread]::CurrentThread.CurrentUICulture = '';& '%~dp0run.ps1' default-build %*; exit $LASTEXITCODE"


I'm just going to assume you copied the bootstrappers wholesale from aspnet/BuildTools and not review them by hand. If you didn't copy them from there please double check that they're identical.

Yes, I took them directly from aspnet/BuildTools.

ryanbrandenburg · 2018-01-22T17:55:00Z

build/repo.props

+    <DotNetCoreRuntime Include="2.1.0-preview1-26016-05" />
+  </ItemGroup>
+
+  <Target Name="GetBuildNumber">


Other repos shouldn't be doing the time-based build number by default now. Is this specific to Webhooks for some reason or did you get it from another repo?

This isn't called by default. It's a repo-specific target people can use.

I find this target incredibly useful because I often install WebHooks packages locally and use them in other projects. It also helps to work around issues like NuGet/Home#6379 and create a self-consistent set of packages.

The pattern is:

> git clean -dfx ... > .\build.cmd /t:GetBuildNumber /nologo Using KoreBuild 2.1.0-preview1-15678 Adding C:\Users\dougbu\.dotnet\x64 to PATH .NET Core runtime 2.0.5 is already installed. Skipping installation. .NET Core SDK 2.2.0-preview1-007947 is already installed. Skipping installation. >>> dotnet.exe msbuild @C:\dd\dnx\Universe++\WebHooks\artifacts\logs\msbuild.rsp /p:BuildNumber=t005c1c2fc Build succeeded. 0 Warning(s) 0 Error(s) Time Elapsed 00:00:01.02 > .\build.cmd /p:BuildNumber=t005c1c2fc /nologo ...

repo.targets then.

ryanbrandenburg · 2018-01-22T17:57:56Z

src/Directory.Build.props

-
-  <PropertyGroup Condition="$(_ImportParentFile)">
-    <DefineConstants>$(DefineConstants);ASPNETWEBHOOKS</DefineConstants>
+  <PropertyGroup>
    <GenerateDocumentationFile>true</GenerateDocumentationFile>
    <RootNamespace>Microsoft.AspNetCore.WebHooks</RootNamespace>


Why do we need to set the RootNamespace to be the same for all the packages?

Need? The default namespace is the same in all projects (except samples). This avoids either repetitive edits as we add classes (if I left <RootNamespace> out entirely) or boilerplate (if <RootNamespace> were in all the projects).

ryanbrandenburg · 2018-01-22T17:59:25Z

src/Directory.Build.props

-
-  <PropertyGroup Condition="$(_ImportParentFile)">
-    <DefineConstants>$(DefineConstants);ASPNETWEBHOOKS</DefineConstants>
+  <PropertyGroup>
    <GenerateDocumentationFile>true</GenerateDocumentationFile>


We should make a point of moving the GenerateDocumentationFile property in other repos up into src/Directory.Build.props as you've done here. Less boilerplate in csproj files and gives us the desired result by default 👍

ryanbrandenburg · 2018-01-22T18:02:17Z

test/Directory.Build.props

  <PropertyGroup>
-    <_ImportParentFile>false</_ImportParentFile>
-    <_ImportParentFile Condition="$(MSBuildProjectName.StartsWith( 'Microsoft.AspNetCore.' ))">true</_ImportParentFile>
+    <DeveloperBuildTestTfms Condition=" '$(DeveloperBuildTestTfms)' == '' ">netcoreapp2.1</DeveloperBuildTestTfms>


Does WebHooks have a particular need for that functionality? If not what's the particular use case you see for this?

ryanbrandenburg

Approved once a couple minor tweaks go in.

dougbu · 2018-01-23T00:15:08Z

25473ff

- #231 - avoid returning a `ValueTuple` and hitting dotnet/standard#567 - reenable tests skipped in #232

dougbu assigned ryanbrandenburg Jan 20, 2018

dougbu requested review from pranavkm, natemcmaster and ryanbrandenburg January 20, 2018 04:13

dougbu added 2 commits January 19, 2018 21:40

Forgot the Readme

767ca8c

Found a few more words

54ce7a0

dougbu added 3 commits January 19, 2018 21:57

Add Travis link to Readme

6b1f395

- should be working soon-ish

Oh great, Travis is running

7544c8d

- get builds there working

Fix CI builds

8617bc6

- build.sh file needs to be executable - not ready for .NET Core App 2.1 with current dependencies

dougbu commented Jan 20, 2018

View reviewed changes

dougbu mentioned this pull request Jan 20, 2018

Move ASP.NET WebHooks code to new aspnet/AspNetWebHooks repo #228

Closed

13 tasks

Remove .NET Core App 2.1 restriction

31e0216

- TFMs weren't lining up

dougbu commented Jan 20, 2018

View reviewed changes

Remove CodeAnalysis configuration from solution

78b10a1

dougbu added a commit that referenced this pull request Jan 21, 2018

Make adjustments after rebase on #232 branch

e02446f

dougbu mentioned this pull request Jan 21, 2018

Add Trello receiver #225

Closed

dougbu added a commit that referenced this pull request Jan 21, 2018

Make adjustments after rebase on #232 branch

adf1deb

dougbu mentioned this pull request Jan 21, 2018

Add WordPress receiver #226

Merged

dougbu added a commit that referenced this pull request Jan 21, 2018

Leftover adjustment to #232

d37deae

ryanbrandenburg suggested changes Jan 22, 2018

View reviewed changes

ryanbrandenburg approved these changes Jan 22, 2018

View reviewed changes

dougbu closed this Jan 23, 2018

dougbu deleted the dougbu/in.the.small branch January 23, 2018 02:28

dougbu added a commit that referenced this pull request Jan 23, 2018

Make adjustments after rebase on #232 branch

23969dd

dougbu added a commit that referenced this pull request Jan 23, 2018

Leftover adjustment to #232

f4b588d

dougbu added a commit that referenced this pull request Jan 24, 2018

Make adjustments after rebase on #232 branch

f450d1c

dougbu added a commit that referenced this pull request Jan 24, 2018

Leftover adjustment to #232

db2fd4e

dougbu added a commit that referenced this pull request Jan 24, 2018

SlackCommand.ParseParameters(...) -> TryParseParameters(...)

e65d214

- #231 - avoid returning a `ValueTuple` and hitting dotnet/standard#567 - reenable tests skipped in #232

dougbu mentioned this pull request Jan 24, 2018

SlackCommand.ParseParameters(...) -> TryParseParameters(...) #234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align with other ASP.NET Core projects #232

Align with other ASP.NET Core projects #232

dougbu commented Jan 20, 2018

dougbu commented Jan 20, 2018

dougbu commented Jan 20, 2018

dougbu Jan 20, 2018

dougbu Jan 20, 2018

dougbu Jan 20, 2018

dougbu Jan 20, 2018

ryanbrandenburg Jan 22, 2018

dougbu Jan 22, 2018

ryanbrandenburg left a comment

ryanbrandenburg Jan 22, 2018 •

edited

Loading

dougbu Jan 22, 2018

ryanbrandenburg Jan 22, 2018

dougbu Jan 22, 2018

ryanbrandenburg Jan 22, 2018

ryanbrandenburg Jan 22, 2018

dougbu Jan 22, 2018

ryanbrandenburg Jan 22, 2018

dougbu Jan 22, 2018

ryanbrandenburg Jan 22, 2018

ryanbrandenburg Jan 22, 2018

dougbu Jan 22, 2018

ryanbrandenburg Jan 22, 2018

ryanbrandenburg Jan 22, 2018

dougbu Jan 22, 2018

ryanbrandenburg Jan 22, 2018 •

edited

Loading

dougbu Jan 22, 2018

ryanbrandenburg Jan 22, 2018

ryanbrandenburg Jan 22, 2018

dougbu Jan 22, 2018

ryanbrandenburg Jan 22, 2018

ryanbrandenburg Jan 22, 2018

ryanbrandenburg left a comment

dougbu commented Jan 23, 2018


		<Import Project="version.props" />
		<Import Project="build\sources.props" />

Align with other ASP.NET Core projects #232

Align with other ASP.NET Core projects #232

Conversation

dougbu commented Jan 20, 2018

dougbu commented Jan 20, 2018

dougbu commented Jan 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg left a comment

Choose a reason for hiding this comment

ryanbrandenburg Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg left a comment

Choose a reason for hiding this comment

dougbu commented Jan 23, 2018

ryanbrandenburg Jan 22, 2018 •

edited

Loading

ryanbrandenburg Jan 22, 2018 •

edited

Loading