-
Notifications
You must be signed in to change notification settings - Fork 191
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 support for source-build #620
Conversation
8c5280d
to
6ca0675
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mainly questioning the need for the
...AspNetCore.Razor.Language/Components/Microsoft.AspNetCore.Components.Razor.Extensions.csproj
Show resolved
Hide resolved
@@ -107,7 +107,7 @@ namespace Microsoft.AspNetCore.Razor.Design.IntegrationTests | |||
|
|||
</Target> | |||
|
|||
<Target Name="RestoreTestProjects" BeforeTargets="Restore;Build"> | |||
<Target Name="RestoreTestProjects" BeforeTargets="Restore;Build" Condition="'$(DotNetBuildFromSource)' != 'true'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test project. Why is it restoring or building at all when '$(DotNetBuildFromSource)' == 'true'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand why this project exists at all either when the projects could just be restored normally. It causes a double restore. @pranavkm
This change is necessary because the "Build" target still runs on all projects in a source build, but with dummy targets that are overrriden to no-op if ExcludeFromSourceBuild is true. See https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Arcade.Sdk/tools/Empty.targets. Because this project file is using "BeforeTargets" to chain in, it too needs the exclusion to avoid restoring projects that are supposed to be ignored in a source build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when the projects could just be restored normally
Just a regular project reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or added to the .sln file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, especially because it's building 😺 My un-resolved suggestions stand (but they're suggestions).
\n\nCommit migrated from dotnet/razor@986c224
\n\nCommit migrated from dotnet/razor@986c224
\n\nCommit migrated from dotnet/razor@986c224 Commit migrated from dotnet/aspnetcore@b45c32fa5b3c
\n\nCommit migrated from dotnet/razor@986c224 Commit migrated from dotnet/aspnetcore@5d1b6db33f6b
Resolves dotnet/aspnetcore#7282
Changes:
build.cmd -test
. We don't run tests in source-build, so this needs to be disabled.cc @dseefeld @crummel