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

Convert SignalR projects to build with ProjectReference #6457

Merged
merged 1 commit into from
Jan 9, 2019
Merged

Convert SignalR projects to build with ProjectReference #6457

merged 1 commit into from
Jan 9, 2019

Conversation

natemcmaster
Copy link
Contributor

@natemcmaster natemcmaster commented Jan 7, 2019 •
edited
Loading

Part of #4246

Changes:

  • Update source code layout to follow the new conventions for this repo
  • Update project files to use <Reference>
  • Update targets to build NPM packages

Reviewers: it's often easier to see the layout changes by looking at the code itself: https://github.com/natemcmaster/AspNetCore/tree/signalr-refs/src/SignalR

@mikaelm12
Copy link
Contributor

Update project files to use

...

@natemcmaster
Copy link
Contributor Author

Update projects to use (invisible xml tags)

JunTaoLuo
JunTaoLuo approved these changes Jan 7, 2019
View reviewed changes
Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with the SignalRisms but the project layout and build/eng updates look good. :shipit: if it builds and tests pass.

BrennanConroy
BrennanConroy reviewed Jan 8, 2019
View reviewed changes
eng/ProjectReferences.props Outdated Show resolved Hide resolved
eng/ProjectReferences.props
@@ -124,5 +124,16 @@
<ProjectReferenceProvider Include="Microsoft.AspNetCore.Authentication.AzureADB2C.UI" ProjectPath="$(RepositoryRoot)src\Azure\AzureAD\Authentication.AzureADB2C.UI\src\Microsoft.AspNetCore.Authentication.AzureADB2C.UI.csproj" />
<ProjectReferenceProvider Include="Microsoft.AspNetCore.AzureAppServices.HostingStartup" ProjectPath="$(RepositoryRoot)src\Azure\AzureAppServices.HostingStartup\src\Microsoft.AspNetCore.AzureAppServices.HostingStartup.csproj" />
<ProjectReferenceProvider Include="Microsoft.AspNetCore.AzureAppServicesIntegration" ProjectPath="$(RepositoryRoot)src\Azure\AzureAppServicesIntegration\src\Microsoft.AspNetCore.AzureAppServicesIntegration.csproj" />
<ProjectReferenceProvider Include="Microsoft.AspNetCore.SignalR.Client.Core" ProjectPath="$(RepositoryRoot)src\SignalR\clients\csharp\Client.Core\src\Microsoft.AspNetCore.SignalR.Client.Core.csproj" />
<ProjectReferenceProvider Include="Microsoft.AspNetCore.SignalR.Client" ProjectPath="$(RepositoryRoot)src\SignalR\clients\csharp\Client\src\Microsoft.AspNetCore.SignalR.Client.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge fan of this naming, but I don't have anything to offer except "SignalR.Client"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming is hard. I figured its presence under src/SignalR/ was sufficient, but I don't care either way. If there is something you'd prefer instead, lmk.

@natemcmaster
Copy link
Contributor Author

Updated to react to dropping netcoreapp2.0 test TFMs. I'd like to merge this later today. PTAL.

@natemcmaster natemcmaster mentioned this pull request Jan 8, 2019
Merged
@natemcmaster
Copy link
Contributor Author

I think something in the SignalR tests is causing the Linux builds to run out of memory, or interfering with the network connection between agent and host. Back when SignalR was in its own repo, did you run into problems like this?

@BrennanConroy
Copy link
Member

Nope, never seen that

@natemcmaster
Copy link
Contributor Author

About to push a big update to this PR. At least one of the problems we have now is that the repo targets implemented to run npm pack always run, even if you are using the build.cmd script in a different subfolder.

The fix for this is to replace those targets with a shim, like a .npmproj file, which invokes npm build/test/ci etc.

To unblock this, we need: aspnet/BuildTools#917

@natemcmaster natemcmaster added the blocked The work on this issue is blocked due to some dependency label Jan 9, 2019
@natemcmaster natemcmaster mentioned this pull request Jan 9, 2019
Merged
@natemcmaster natemcmaster removed the blocked The work on this issue is blocked due to some dependency label Jan 9, 2019
@natemcmaster
Copy link
Contributor Author

Updated BuildTools. Should be unblocked now.

Convert SignalR projects to build with ProjectReference
956c2ca 
Changes:
* Update source code layout to follow the new conventions for this repo
* Update project files to use <Reference>
* Update targets to build NPM packages
BrennanConroy
BrennanConroy approved these changes Jan 9, 2019
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Don't know what's up with the build

@natemcmaster natemcmaster merged commit d383862 into dotnet:release/2.1 Jan 9, 2019
@natemcmaster natemcmaster deleted the signalr-refs branch January 9, 2019 21:12
@natemcmaster
Copy link
Contributor Author

I'll handle kestrel's test failures separately.

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

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@JunTaoLuo JunTaoLuo JunTaoLuo approved these changes

@mikaelm12 mikaelm12 Awaiting requested review from mikaelm12

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

Successfully merging this pull request may close these issues.
4 participants
@natemcmaster @mikaelm12 @BrennanConroy @JunTaoLuo