Use makefile.proj if available #158
Conversation
|
As an alternative, consider adding a msbuild project into KoreBuild that conditionally imports Concept: <Project>
<Target Name="Build" />
<Import Project="$(RepoDir)\makefile.targets" Condition="Exists('$(RepoDir)\makefile.targets')" />
</Project>in repo <Project>
<Target Name="AfterBuild" AfterTargets="Build" />
</Project> |
|
build/makefile.proj if the file in the root affects builds? Presumably that solves and moves more build related files to the build dir. |
pranavkm
force-pushed
the
prkrishn/run-makefile
branch
from
2d680b8 to
1dc95a7
Compare
|
I like the idea of putting it in the build/ folder. How about build/repo.targets ? |
pranavkm
force-pushed
the
prkrishn/run-makefile
branch
from
1dc95a7 to
f1eeadd
Compare
|
🆙 📅 with corresponding changes to https://github.com/aspnet/CoreCLR/pull/149/ |
| @@ -0,0 +1,17 @@ | |||
| <Project> | |||
There was a problem hiding this comment.
Should this be set to DefaultTargets="Default"?
There was a problem hiding this comment.
This gets included by makefile.proj which has the DefaultTarget. Would we need one here?
There was a problem hiding this comment.
nope, you're right. I just didn't see that
| </PropertyGroup> | ||
|
|
||
| <Exec | ||
| Command="$(SakeExecutable) -I $(KoreBuildDirectory)shade -f $(MakeFilePath) $(ShadeTarget)" |
There was a problem hiding this comment.
I did this exact same work a few weeks ago (bu never finished it) and ran into issues with relative vs absolute paths. Sake.exe didn't recognize absolute paths for the -f argument. Have you run into this issue here?
There was a problem hiding this comment.
KoreBuildDirectory is root relative - notice I had to concat the path on Line 145 with RepositoryRoot to make it absolute. Btw, this brought up an interesting issue with using back slashes in our code - the things we invoke on xplat don't like back slashes (for instance mono build\sake.exe). Calling Path.GetFullPath helps, but it'd be much better to standardize on forward slashes in our targets.
There was a problem hiding this comment.
gotcha. was just checking :)
build/KoreBuild.ps1
Outdated
| if ($allparams) { | ||
| $targets = '/t:' + $allparams.Replace(' ', ';') | ||
| } | ||
| dotnet msbuild $makeFileProj /p:KoreBuildDirectory="$koreBuildFolder\" /p:RepositoryRoot="$repoFolder\" $targets |
There was a problem hiding this comment.
$targets needs to be surrounded with double quotes so multiple targets doesn't terminate the line.
Suggested adding these parameters:
/maxcpucount /fileLogger /fileloggerparameters:LogFile=artifacts/msbuild.log /detailedsummary
Also, consider making an rsp file (artifacts/msbuild.rsp) and executing "dotnet msbuild @artifacts/msbuild.rsp" instead.
Nit: add newline at end of file.
There was a problem hiding this comment.
I actually tried multiple targets and it works fine - primarily because the $targets is a variable and not code in the file with semicolons.
There was a problem hiding this comment.
glad this PowerShell quirk works in our favor.
pranavkm
force-pushed
the
prkrishn/run-makefile
branch
4 times, most recently
from
e20171a to
fb71594
Compare
pranavkm
force-pushed
the
prkrishn/run-makefile
branch
from
fb71594 to
0e9f203
Compare
|
🆙 📅 |
| $targets | ||
| ENDMSBUILDARGS | ||
|
|
||
| dotnet msbuild @"$msbuildResponseFile" |
There was a problem hiding this comment.
One more thought. Right now this converts $@ to a list of targets. How would we go about flowing through properties? e.g. /p:Configuration=Release.
|
🔔 can we get this in soon? I want to rebase #159 off of this work. |
|
Merged. Let's see how this goes. |
|
Ok - need to tweak this so that it allows calling targets that aren't declared in KoreBuild (for instance Universe has different targets). I'll play around with this and resend. |
|
I'll send an update to this later. |
No description provided.