-
Notifications
You must be signed in to change notification settings - Fork 23
Build the sln file instead of individual project files #159
Conversation
build/shade/_k-standard-goals.shade
Outdated
} | ||
|
||
if (projectGlobs.Any()) | ||
foreach (var sln in Files.Include(Path.Combine(BASE_DIR, "*.sln"))) |
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.
Could we just build $repoName.sln
- seems like a convention all our repos follow. We could throw an error if there happen to be more than one sln.
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.
Yeah, that could be fine. I know off the top of my head that Diagnostics has DiagnosticPages.sln. Are there others?
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.
HttpSysServer still has WebListener.sln though I think WebListener as a repo name will redirect properly
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.
We also need to change that rename in aspnet/Universe's list of repos.
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.
We could throw an error if there happen to be more than one sln.
Meant to say we could throw an error to say we couldn't find the sln if we don't find it.
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.
The only problematic thing is...how do you determine "$repoName"? We can't use directory name as it's not guaranteed....
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.
That's true. I guess we should get Mvc to move the second sln (the NoFun one) out of the repo root as part of the migration. We'll switch to calling dotnet build
and have it fail in the non-common cases.
FYI @NTaylorMullen
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.
Bummer, was working around some issues with having multiple sln's in Mvc's migrated makefile. Could we take the approach of having a parameter that we can set to override the default sln name?. It'd be a shame to hide sln's that we actually use from users who clone our repos.
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.
Instead of a parameter, we could build a well known file (build.proj
?) if it's present. We can't run dotnet build
regardless.
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 think this problem can be solved by the work Pranav is doing on #158. The default Korebuild build step could include:
<SolutionsToBuild Include="$(RepoRoot)\*.sln" />
MVC and other repos could customize by adding exclusions to the build/repo.targets
file:
<SolutionsToBuild Remove="$(RepoRoot)\Mvc.NoFun.sln" />
13c1459
to
7228b4e
Compare
Almost there: something in Korebuild is attempting to use obj/*.dll...I think.
Doesn't happen when you call "dotnet build" on the solution, w/o korebuild |
Nvm, it's dotnet/sdk. dotnet/sdk#739 |
</PropertyGroup> | ||
|
||
<Exec | ||
Command="$(SakeExecutable) -I $(KoreBuildDirectory)shade -f $(MakeFilePath) $(ShadeTarget)" | ||
Command="$(SakeExecutable) -I "$(MSBuildThisFileDirectory)../shade" -f $(MakeFilePath) $(SakeTargets)" |
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.
👏
build/targets/common.props
Outdated
@@ -1,6 +1,7 @@ | |||
<Project> | |||
<PropertyGroup> | |||
<BuildDir>$(RepositoryRoot)\build</BuildDir> | |||
<ArtifactsDir>$(BuildDir)\artifacts</ArtifactsDir> | |||
<RepositoryRoot Condition="'$(RepositoryRoot)'==''">$(MSBuildThisFileDirectory)..\..\</RepositoryRoot> |
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 location can easily be incorrect, especially in (nested) test directories. Why isn't this using $([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), "$(Repository).sln")
or something similar?
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.
MSBuildThisFileDirectory refers to the location of .build/targets/common.props, not the location of test projects. This value isn't affect by nested test directories.
|
||
<Target Name="Clean" /> | ||
<ItemGroup> | ||
<SolutionsToBuild Include="$(RepositoryRoot)\*.sln" /> |
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 won't work well at least for Mvc. Why not $(RepositoryRoot)\$(RepoName).sln
?
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.
The goal here is to make the default case work, which is that we have one sln per repo. MVC and any other solution that has multiple solutions would put something like this in to build/repo.targets:
<SolutionsToBuild Remove="Mvc.NoFun.sln" />
We don't have a good way to determine "RepoName" :-/ as the folder name containing the source code is not guaranteed to match the sln name.
build/shade/_k-standard-goals.shade
Outdated
{ | ||
Dotnet("msbuild /nologo" | ||
+ " \"" + Path.Combine(Directory.GetCurrentDirectory(), ".build/targets/makefile.proj") + "\"" | ||
+ " \"/p:LifecycleType=Standard;Configuration=" + E("Configuration") + "\"" |
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.
With changes to build/targets/makefile.proj
, does this command-line lifecycle choice still work?
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.
For now, yes, you can still express build verify
and build --for-ci build-compile
etc.
af694f7
to
d7c5079
Compare
🆙 📅 Removing the [blocked] label. The race condition in solution builds has been fixed. |
I added two changes to be aware of. (1) the tool dependencies (Sake, NuGetPackageVerifier, and NETFrameworkReferenceAssemblies) restore into the regular nuget cache instead of into the .build folder We'll need to update other repos. Any project that has multiple *.sln files or has projects in the solution file that doen't build with dotnet-CLI will need to be updated. Razor, MVC, and possibly others. |
d7c5079
to
8758c60
Compare
This reverts commit 1cc9f90.
8758c60
to
4778c94
Compare
cc @pakrym |
Wouldn't calling pack on each project try to rebuild everything again? |
At the moment yes it will build twice. This is an incremental step towards eliminating the Sake lifecycle. Some of our repos have custom packaging steps that we need to port to MSBuild. This has the advantage of only rebuilding projects once during the pack phase. MSBuild will batch the builds during pack. In a follow up PR, we can move even more into MSBuild so each project should only compile once per build.cmd run. |
👍 LGTM |
4778c94
to
6616302
Compare
Perf improvement: only builds each project once during the 'build-compile' target.