[PATCH] D127938: Update Windows packaging script.

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: llvm/utils/release/build_llvm_release.bat:6
+REM
+REM Run the script with administrator permissions.
+if not exist %SYSTEMROOT%\SYSTEM32\WDI\LOGFILES (
----------------
Wait, what? This seems undesirable.


================
Comment at: llvm/utils/release/build_llvm_release.bat:22
 REM   NSIS with the strlen_8192 patch,
-REM   Visual Studio 2019 SDK and Nuget (for the clang-format plugin),
-REM   Perl (for the OpenMP run-time).
-REM
+REM   Visual Studio 2019 SDK
+REM   Perl (for the OpenMP run-time), 7-Zip.
----------------
Unrelated to this patch, but since we no longer build the clang-format plugin, this SDK is no longer needed.


================
Comment at: llvm/utils/release/build_llvm_release.bat:48
+set "p64="
+set "output="
+set "trace="
----------------
"output" isn't a great variable name. How about something like "build_dir_base"?


================
Comment at: llvm/utils/release/build_llvm_release.bat:61
+set location=https://github.com/llvm/llvm-project/archive/refs/heads/main.zip
+set "revision=99.9.9"
+
----------------
thieta wrote:
> Revision and version seems to be used interchangeable here - I would call it version when it's dealing with versions and revision if you extract the Git sha.
+1, "version" should be what gets fed to the PACKAGE_VERSION cmake variable, and "revision" should be the git revision.


================
Comment at: llvm/utils/release/build_llvm_release.bat:63
+
+if defined version (
+  if not "%version%"=="true" (
----------------
I haven't seen batch syntax like this before. I guess it's new?


================
Comment at: llvm/utils/release/build_llvm_release.bat:112
   -DCMAKE_CL_SHOWINCLUDES_PREFIX="Note: including file: " ^
-  -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;lld;compiler-rt;lldb;openmp"
+  -DLLVM_LIT_ARGS="-s" ^
+  -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;lld;compiler-rt;openmp;lldb"
----------------
What's this for?


================
Comment at: llvm/utils/release/build_llvm_release.bat:113
+  -DLLVM_LIT_ARGS="-s" ^
+  -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;lld;compiler-rt;openmp;lldb"
+
----------------
Any reason for reordering the projects here?


================
Comment at: llvm/utils/release/build_llvm_release.bat:115
+
+REM Build the 32-bits and/or 64-bits binaries.
+if defined b32 call :build_32
----------------
This is a good idea. We may want to drop the 32-bit packages from releases eventually.


================
Comment at: llvm/utils/release/build_llvm_release.bat:125
+REM Preserve original path
+set original_path=%PATH%
+
----------------
Maybe we should move this closer to the top, and do PATH=%original_path% at the start of build_32 and build_64 instead? That seems less brittle.


================
Comment at: llvm/utils/release/build_llvm_release.bat:145
+cmake -GNinja %cmake_flags% ..\llvm-project\llvm || exit /b 1
+%build_tool% all || exit /b 1
+
----------------
As Tobias mentioned already, please keep the re-tries for all build steps and test invocations. Build environments are sometimes unreliable due to antivirus software etc. and retrying individual steps is much faster than retrying the whole script.


================
Comment at: llvm/utils/release/build_llvm_release.bat:184
+
+  7z x -y LLVM-%package_version%-win32.exe -orepack
+  rmdir /s /q repack\$PLUGINSDIR
----------------
Since it takes quite a while to run, can this repackaging step be put behind a flag?


================
Comment at: llvm/utils/release/build_llvm_release.bat:261
+
+  7z x -y LLVM-%package_version%-win64.exe -orepack
+  rmdir /s /q repack\$PLUGINSDIR
----------------
thieta wrote:
> why is the exe zipped after created?
> 
> in another suggestion (doesn't need to be addressed here) I think we probably should upload a zip file in addition to the .exe so that people that repack the binaries doesn't have to install it to unpack it.
The old script did produce .zip files for a while, but I never uploaded them and eventually removed it because the files were very large, it made the script slower, and people who want to repack the binaries can already extract from the installer with 7zip without running it.


================
Comment at: llvm/utils/release/build_llvm_release.bat:278
+:run_tests
+  %build_tool% check
+  %build_tool% check-clang
----------------
thieta wrote:
> Why not `check-all` here?
> 
> I also think that output from the testing should be saved to a logfile - similar to our release-test script. That makes it possible to check which tests that failed when running from a CI or similar without having to search the output.
I'm pretty sure check-lldb wouldn't pass. I'm not even sure all these tests pass, note that some of them were commented out for 32-bit builds previously.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127938/new/

https://reviews.llvm.org/D127938