[PATCH] D127938: Update Windows packaging script.

[Douglas Yung via Phabricator via llvm-commits]

dyung added inline comments.


================
Comment at: llvm/utils/release/build_llvm_release.bat:2
 @echo off
 setlocal
 
----------------
I know you didn't change this, but when I write batch files, I generally reverse these two options, because I believe in this order, the `@echo off` would persist after the script exits which I don't like scripts changing my environment unecessarily.


================
Comment at: llvm/utils/release/build_llvm_release.bat:52
+if not defined base-dir (
+  set "base-dir=%TEMP%"
+  if not defined base-dir (
----------------
I've not really seen this method of using set before (with the double quotes at the beginning and the end) and I'm not sure that it would handle paths with spaces correctly. For example, I wrote the following test script:
```
set TEST1="C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\bin\Hostx64\x64\cl.exe"
set "TEST2=C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\bin\Hostx64\x64\cl.exe"

echo TEST1=%TEST1%
echo TEST2=%TEST2%
```
Both set a variable to the path to cl.exe and then I print out the value. The output looks like this:
```
TEST1="C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\bin\Hostx64\x64\cl.exe"
TEST2=C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\bin\Hostx64\x64\cl.exe
```
The difference is significant because later on in the script you use `%base-dir%` without enclosing it in double quotes, so if there are spaces or parenthesis in the path, it could potentially cause problems with the script.


================
Comment at: llvm/utils/release/build_llvm_release.bat:58
+    echo Missing base-dir directory.
+    exit /b
+  )
----------------
Is this missing the return value you intend to exit with?


================
Comment at: llvm/utils/release/build_llvm_release.bat:62
+REM Validate base-dir directory.
+if not exist %base-dir% mkdir %base-dir%
+cd /d %base-dir%
----------------
I recommend always enclosing variables that could contain a path in double quotes to deal with possible spaces or parenthesis in the path.


================
Comment at: llvm/utils/release/build_llvm_release.bat:68
+if defined version (
+  if not "%version%"=="true" (
+    set location=https://github.com/llvm/llvm-project/archive/llvmorg-%version%.zip
----------------
For string comparisons in batch files, unless you need to check for cases of the arguments, I generally recommend adding `/I` to make it case insensitive.


================
Comment at: llvm/utils/release/build_llvm_release.bat:178
+  -DCMAKE_CXX_FLAGS="-Wno-nonportable-include-path" ^
+  -DCMAKE_C_COMPILER=%build_dir%/build32_stage0/bin/clang-cl.exe ^
+  -DCMAKE_CXX_COMPILER=%build_dir%/build32_stage0/bin/clang-cl.exe ^
----------------
Will this handle the case where `%build_dir%` contains a space or parenthesis?


================
Comment at: llvm/utils/release/build_llvm_release.bat:182
+  -DCMAKE_AR=%build_dir%/build32_stage0/bin/llvm-lib ^
+  -DCMAKE_RC=%build_dir%/build32_stage0/bin/llvm-windres
+set cmake_flags=%all_cmake_flags:\=/%
----------------
For consistency, is there a reason why CMAKE_AR and CMAKE_RC do not specify the tool with .exe at the end?


================
Comment at: llvm/utils/release/build_llvm_release.bat:372
+  echo.
+  echo Usage: build_llvm_release.bat ^<options^>
+  echo.
----------------
Instead of hardcoding the script name here, I think we should just use `%0` or `%~0` (in case it may have double quotes around it).


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

https://reviews.llvm.org/D127938