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

Convert Components projects to use ProjectRef #6698

Merged
merged 20 commits into from
Jan 16, 2019

Conversation

natemcmaster
Copy link
Contributor

This addresses #4246 for src/Components/. I've implemented the source code layout as described here: https://github.com/aspnet/AspNetCore-Internal/issues/1535#issuecomment-451488620

This PR is WIP because it doesn't build yet. I'll make comments about the problems I've run into.

@natemcmaster natemcmaster requested a review from pranavkm January 15, 2019 22:00
@natemcmaster
Copy link
Contributor Author

I'm getting a weird error attempting to build Microsoft.VisualStudio.BlazorExtension.csproj.

error NETSDK1050: The version of Microsoft.NET.Sdk used by this project is insufficient

It doesn't appear this project is currently building anyways, so I've excluded it. Is that okay?

@SteveSandersonMS
Copy link
Member

I'm getting a weird error attempting to build Microsoft.VisualStudio.BlazorExtension.csproj.

OK, interesting. We do need to be building that VSIX (at least by this time next week). I don't know what that error is about or how to address it, but if it has to be handled after this PR that's probably viable.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jan 15, 2019

@natemcmaster As part of this, can I check you're able to run the E2E tests locally and that they all pass? They aren't running in CI yet because we still have unresolved issues with getting the Chrome and Java into the CI environments, but they do run locally. It's easiest to run them from VS.

@natemcmaster
Copy link
Contributor Author

As part of this, can I check you're able to run the E2E tests locally and that they all pass?

Sure, once I can get this compiling. I'm getting a compiler error in generated code that I don't understand, possibly the result of the change in layout of files. Any ideas @pranavkm ?

C:\src\aspnet\AspNetCore\src\Components\Blazor\perf\obj\Debug\netstandard2.0\RazorDeclaration\App.cshtml.g.cs(21,54): error CS0234: The type or namespace name 'Shared' does not exist in the namespace 'Microsoft.AspNetCore.Blazor.E2EPerformance' (are you missing an assembly reference?) [C:\src\aspnet\AspNetCore\src\Components\Blazor\perf\Microsoft.AspNetCore.Blazor.E2EPerformance.csproj]

@SteveSandersonMS
Copy link
Member

possibly the result of the change in layout of files

Yes, I think that's it. Currently the Razor Components compiler uses the folder name as the generated namespace, so your change to put them in a folder called perf will have broken the namespaces. Unfortunately we can't use that folder layout with Razor Components projects currently.

@natemcmaster
Copy link
Contributor Author

Can we not do this? I understand this might just be one step trying to move towards getting it to build, but I'd be extremely keen not to have bizarre namespaces in the code!

Is there an option to tell the razor compiler to use a different generated namespace? It appears it does not honor RootNamespace.

@natemcmaster
Copy link
Contributor Author

One of the unit tests is failing because there is a new file in output: System.Diagnostics.Tracing.dll.

Failed   Microsoft.AspNetCore.Blazor.Build.Test.RuntimeDependenciesResolverTest.FindsReferenceAssemblyGraph_ForStandaloneApp
Error Message:
 Assert.Equal() Failure
Expected: String[] ["Microsoft.AspNetCore.Blazor.dll", "Microsoft.AspNetCore.Blazor.pdb", "Microsoft.AspNetCore.Components.Browser.dll", "Microsoft.AspNetCore.Components.Browser.pdb", "Microsoft.AspNetCore.Components.dll", ...]
Actual:   String[] ["Microsoft.AspNetCore.Blazor.dll", "Microsoft.AspNetCore.Blazor.pdb", "Microsoft.AspNetCore.Components.Browser.dll", "Microsoft.AspNetCore.Components.Browser.pdb", "Microsoft.AspNetCore.Components.dll", ...]
Stack Trace:
   at Microsoft.AspNetCore.Blazor.Build.Test.RuntimeDependenciesResolverTest.FindsReferenceAssemblyGraph_ForStandaloneApp() in C:\src\aspnet\AspNetCore\src\Components\Blazor\Build\test\RuntimeDependenciesResolverTest.cs:line 133

It appears the problem is that this file is in output, but not listed as expected: "System.Diagnostics.Tracing.dll". Should I add this to the list of expected files?

Also, I tried to start selenium so I could run E2E Tests, but it fails to boot on my box. I have the JDK 11 installed.

Error: opening registry key 'Software\JavaSoft\Java Runtime Environment'
Error: could not find java.dll
Error: Could not find Java SE Runtime Environment.
C:\Users\namc\AppData\Roaming\npm\node_modules\selenium-standalone\bin\selenium-standalone:79
        throw err;
        ^

Error: Selenium exited before it could start
Another Selenium process may already be running or your java version may be out of date.
Be sure to check the official Selenium release notes for minimum required java version: https://raw.githubusercontent.com/SeleniumHQ/selenium/master/java/CHANGELOG

@javiercn
Copy link
Member

Also, I tried to start selenium so I could run E2E Tests, but it fails to boot on my box. I have the JDK 11 installed.

I think you need a lower JDK as it doesn't like 11. But @SteveSandersonMS can confirm

@natemcmaster
Copy link
Contributor Author

JDK < 11 has gone out of support on Windows. I don't think we should require people to install JDKs which may include vulnerable versions of Java.

@natemcmaster natemcmaster changed the title wip: Convert Components projects to use ProjectRef Convert Components projects to use ProjectRef Jan 15, 2019
@SteveSandersonMS
Copy link
Member

Is there an option to tell the razor compiler to use a different generated namespace

I don't think so, but cc @rynowak

@SteveSandersonMS
Copy link
Member

It appears the problem is that this file is in output, but not listed as expected: "System.Diagnostics.Tracing.dll". Should I add this to the list of expected files?

Yes, I think that's OK. I know it would be tough to trace back precisely why this has changed. If it turns out to be a problem we can investigate further.

@rynowak
Copy link
Member

rynowak commented Jan 16, 2019

I replied elsewhere. @namespace FooBar is that feature. You can stick that in a _ViewImports.cshtml and it will compute namespaces of related files as you would expect.

@natemcmaster
Copy link
Contributor Author

I ended up renaming the "perf" folder back to "Microsoft.AspNetCore.Blazor.E2EPerformance" due to this:

https://github.com/aspnet/AspNetCore/blob/df7bfe52433c19f9ceaf49721201e77db9dab463/src/Components/blazor/src/Microsoft.AspNetCore.Blazor.Cli/Server/Startup.cs#L74

Tests assume assembly name == the directory name of contentRoot. Looks like a product issue that needs to be resolved separately.

@natemcmaster
Copy link
Contributor Author

209 of 214 E2E tests passed locally. The ones failing had this error:

Failed   Microsoft.AspNetCore.Components.E2ETest.Tests.HttpClientTest.CanReadResponseHeaders
Error Message:
 Assert.Equal() Failure
          ↓ (pos 0)
Expected: OK
Actual:   NotAcceptable
          ↑ (pos 0)
Stack Trace:
   at Microsoft.AspNetCore.Components.E2ETest.Tests.HttpClientTest.CanReadResponseHeaders() in C:\src\aspnet\AspNetCore\src\Components\test\E2ETest\Tests\HttpClientTest.cs:line 71
Standard Output Messages:
 browser Logs from Selenium:
 [1/16/2019 1:59:29 AM] - Info - http://127.0.0.1:54797/subdir/_framework/components.webassembly.js 551:91 "WASM: GC_MINOR: (Nursery full) time 2.00ms, stw 2.10ms promoted 103K major size: 976K in use: 220K los size: 0K in use: 0K"
 [1/16/2019 1:59:30 AM] - Severe - http://127.0.0.1:55209/api/person - Failed to load resource: the server responded with a status of 406 (Not Acceptable)

@natemcmaster
Copy link
Contributor Author

🆙 📅

Running src/Components/build.cmd -test passes locally now.

I still get issues when running E2E tests, but those don't appear to be related to my changes.

@natemcmaster
Copy link
Contributor Author

CI checks are green. Okay to merge? I'd like to get this in soon so we ensure preview 2 versions of Components stay up to date with the latest changes made its dependencies.

@SteveSandersonMS
Copy link
Member

@natemcmaster Did you resolve the E2E test issues? They were passing before. I'm not sure how them not passing now could be unrelated to this PR, but please ping me if you would like me to help debug!

Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Project structure looks good. @SteveSandersonMS \ @rynowak to sign off on the change

@natemcmaster
Copy link
Contributor Author

I found the problem and fixed it. The tests were failing because they had not been updated to react to #5146. The server was responsding with HTTP 406 because there was no formatter available to turn string[] into JSON.

You can run all tests locally now by running

build.cmd -test /p:BlazorAllTests=true

cc @pranavkm

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Thanks for all the updates!

@natemcmaster natemcmaster merged commit 5a70f53 into dotnet:master Jan 16, 2019
@natemcmaster natemcmaster deleted the comp-ref branch January 16, 2019 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants