[PATCH] D127938: Update Windows packaging script.

[Tobias Hieta via Phabricator via llvm-commits]

thieta requested changes to this revision.
thieta added a comment.
This revision now requires changes to proceed.

A bunch of questions and comments. I think there are some really nice cleanups here, but also some features / behaviors that change that we need to understand.



================
Comment at: llvm/utils/release/build_llvm_release.bat:38
+REM Specify building tool.
+set build_tool=ninja
+
----------------
This can't realistically be anything but ninja right? is there a point of this indirection?


================
Comment at: llvm/utils/release/build_llvm_release.bat:55
+REM Validate command line options.
+if not defined output (
+  echo Missing output directory.
----------------
I think you can default to a output directory instead of failing.


================
Comment at: llvm/utils/release/build_llvm_release.bat:60
+
+set location=https://github.com/llvm/llvm-project/archive/refs/heads/main.zip
+set "revision=99.9.9"
----------------
Hmm. Instead of hardcoding main here - I think maybe we should take this opportunity to support building any random branch/sha/tag here. Then it will be more flexible instead of build main or released version.


================
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"
+
----------------
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.


================
Comment at: llvm/utils/release/build_llvm_release.bat:241
+  -DCMAKE_CXX_COMPILER=%build_dir%/build64_stage0/bin/clang-cl.exe ^
+  -DCMAKE_LINKER=%build_dir%/build64_stage0/bin/lld-link.exe
+set cmake_flags=%all_cmake_flags:\=/%
----------------
maybe we should use other LLVM tools here as well? `CMAKE_AR=llvm-lib` `CMAKE_RC=llvm-windres` etc.


================
Comment at: llvm/utils/release/build_llvm_release.bat:252
+  call :print_trace_message Build64 Stage1 - Testing
+  call :run_tests
+)
----------------
The retry logic from the old script was lost here - should we at least attempt to test if it fails? I am guessing this was in the original script because there are some transitional errors that can happen?


================
Comment at: llvm/utils/release/build_llvm_release.bat:261
+
+  7z x -y LLVM-%package_version%-win64.exe -orepack
+  rmdir /s /q repack\$PLUGINSDIR
----------------
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.


================
Comment at: llvm/utils/release/build_llvm_release.bat:278
+:run_tests
+  %build_tool% check
+  %build_tool% check-clang
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127938