Add infrastructure for building coredistools #289
Conversation
…ownload-llvm-release.yml
|
@dotnet/jit-contrib PTAL For some reason macOS Azure DevOps hosted pool is super-slow today, so I don't expect the macOS job to succeed. (It was working fine yesterday and the day before). I will open a meta-issue to track down the remaining work, but this PR should a major un-blocker - the binaries are built for all target platforms and all the host platforms that .NET Core supports. |
BruceForstall
left a comment
BruceForstall
left a comment
There was a problem hiding this comment.
LGTM. I only had minor suggestions for improving the scripts (which I realized after writing the comments would probably only be invoked by the YML, not manually, but still...)
| @@ -0,0 +1,66 @@ | |||
| @echo off | |||
| setlocal | |||
There was a problem hiding this comment.
It's useful to use: setlocal EnableDelayedExpansion EnableExtensions
| @@ -0,0 +1,66 @@ | |||
| @echo off | |||
| setlocal | |||
|
|
|||
There was a problem hiding this comment.
I like to see a usage message at the top, which can double as the documentation for what the script does. E.g.,:
@echo off
setlocal
goto start
:usage
echo Usage: build-coredistools.cmd ^<TargetOSArchitecture^>
echo where ^<TargetOSArchitecture^> is one of: windows-arm, windows-arm64, windows-x64, windows-x86.
goto :eof
:start
set RootDirectory=%~dp0
set SourcesDirectory=%RootDirectory%src
set BinariesDirectory=%RootDirectory%obj
set TargetOSArchitecture=%1
if /i "%TargetOSArchitecture%" == "windows-arm" (
set GeneratorPlatform=ARM
) else if /i "%TargetOSArchitecture%" == "windows-arm64" (
set GeneratorPlatform=ARM64
) else if /i "%TargetOSArchitecture%" == "windows-x64" (
set GeneratorPlatform=x64
) else if /i "%TargetOSArchitecture%" == "windows-x86" (
set GeneratorPlatform=Win32
) else (
echo "Unknown target OS and architecture: %TargetOSArchitecture%"
goto usage ///********
)
| set BinariesDirectory=%RootDirectory%obj | ||
| set TargetOSArchitecture=%1 | ||
|
|
||
| if /i "%TargetOSArchitecture%" == "windows-arm" ( |
There was a problem hiding this comment.
Could this default to using PROCESSOR_ARCHITECTURE?
| -DLLVM_TABLEGEN="%LLVMTableGen%" ^ | ||
| -DLLVM_TARGETS_TO_BUILD=AArch64;ARM;X86 ^ | ||
| -DLLVM_TOOL_COREDISTOOLS_BUILD=ON ^ | ||
| "%SourcesDirectory%\llvm-project\llvm" |
There was a problem hiding this comment.
You should check the ERRORLEVEL return for all 3 cmake invocations, not just one.
| ;; | ||
|
|
||
| *) | ||
| echo "Unknown target OS and architecture: $TargetOSArchitecture" |
There was a problem hiding this comment.
A similar comment applies here about it being useful to have a "help" / usage comment and, if necessary and separate, a "what does this script do" header comment.
|
|
||
| pushd "%BinariesDirectory%" | ||
|
|
||
| cmake.exe ^ |
There was a problem hiding this comment.
check all the cmake exit codes?
| @@ -0,0 +1,45 @@ | |||
| @echo off | |||
| setlocal | |||
|
|
|||
There was a problem hiding this comment.
Add a "what does this script do?" header comment?
You could also have:
if not "%1"=="" echo Error: no arguments expected&goto :eof
|
Thanks @BruceForstall for code review! I will have a follow-up PR when I address some of the issues with Arm32 I am working on and all of your suggestions here. However, I would like to have a working build pipeline at the moment when I open that PR, so I am merging these changes. |
No description provided.