[PATCH] D127938: Update Windows packaging script.

Thu Jun 16 23:15:45 PDT 2022

CarlosAlbertoEnciso 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 (
----------------
hans wrote:
> Wait, what? This seems undesirable.
Running the script from a standard DOS prompt, I get the following errors:

```
...
7-Zip 21.06 (x64) : Copyright (c) 1999-2021 Igor Pavlov : 2021-11-24

Scanning the drive for archives:
1 file, 223803667 bytes (214 MiB)

Extracting archive: src.zip
--
Path = src.zip
Type = zip
...
ERROR: Cannot create symbolic link : A required privilege is not held by the client. : .\llvm-project-main\libclc\clspv64
ERROR: Cannot create symbolic link : A required privilege is not held by the client. : .\llvm-project-main\libcxx\test\std\pstl
ERROR: Cannot create symbolic link : A required privilege is not held by the client. : .\llvm-project-main\openmp\tools\analyzer\llvm-openmp-analyzer++

Sub items Errors: 17

Archives with Errors: 1

Sub items Errors: 17
```
May be the configuration we use is incorrect.

================
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.
----------------
hans wrote:
> Unrelated to this patch, but since we no longer build the clang-format plugin, this SDK is no longer needed.
Removed that comment.

================
Comment at: llvm/utils/release/build_llvm_release.bat:38
+REM Specify building tool.
+set build_tool=ninja
+
----------------
thieta wrote:
> This can't realistically be anything but ninja right? is there a point of this indirection?
You are right.
In our case we have an internal tool based on ninja, that uses a different name.
May be use something like:

```
REM Specify building tool and CMake generator
set build_tool=ninja
set cmake_generator=Ninja
``` 

The `cmake_generator` to be used with cmake -G option. That gives the option to use different systems for building.

================
Comment at: llvm/utils/release/build_llvm_release.bat:48
+set "p64="
+set "output="
+set "trace="
----------------
hans wrote:
> "output" isn't a great variable name. How about something like "build_dir_base"?
In the way the command line parsing works, I will keep `output` as the option and `build_dir_base` in the script. Somthing like:

```
set "output="
...
call :parse_args %*

set build_dir_base=%output%
```

================
Comment at: llvm/utils/release/build_llvm_release.bat:55
+REM Validate command line options.
+if not defined output (
+  echo Missing output directory.
----------------
thieta wrote:
> I think you can default to a output directory instead of failing.
May be use the user temporal directory as default value.

================
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"
----------------
thieta wrote:
> 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.
That is a good idea. It gives more flexibility.

Can we put this feature on hold for the next patch, once the basic patch (this one) is done.

================
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"
+
----------------
hans wrote:
> 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.
I will update it.

================
Comment at: llvm/utils/release/build_llvm_release.bat:63
+
+if defined version (
+  if not "%version%"=="true" (
----------------
hans wrote:
> I haven't seen batch syntax like this before. I guess it's new?
For what I know it was introduced back in Windows NT.
But if you prefer, i can use the explicit checks:

```
if not "%version%"==""
```

================
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"
----------------
hans wrote:
> What's this for?
The LLVM_LIT_ARGS are the arguments for the Integreted tester.

https://releases.llvm.org/11.0.0/docs/CommandGuide/lit.html

and we use '-s' to reduce the amount of information generated during testing.

================
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"
+
----------------
hans wrote:
> Any reason for reordering the projects here?
No reason.
Put back the original order.
```
-DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;lld;compiler-rt;lldb;openmp"
```

================
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
----------------
hans wrote:
> This is a good idea. We may want to drop the 32-bit packages from releases eventually.
In our case we build only 64-bits.

================
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
+
----------------
hans wrote:
> 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.
Putting it back the re-tries for all build steps and test invocations.

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

================
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:\=/%
----------------
thieta wrote:
> maybe we should use other LLVM tools here as well? `CMAKE_AR=llvm-lib` `CMAKE_RC=llvm-windres` etc.
Added those tools.

================
Comment at: llvm/utils/release/build_llvm_release.bat:252
+  call :print_trace_message Build64 Stage1 - Testing
+  call :run_tests
+)
----------------
thieta wrote:
> 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?
Restored the retry logic.

================
Comment at: llvm/utils/release/build_llvm_release.bat:278
+:run_tests
+  %build_tool% check
+  %build_tool% check-clang
----------------
hans wrote:
> 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.
I have created specific functions for the tests:
```
:: Run 32-bits tests.
:test_32
  %build_tool% check || %build_tool% check || %build_tool% check || exit /b 1
  ...
exit /b 0

:: Run 64-bits tests.
:test_64
  %build_tool% check || %build_tool% check || %build_tool% check || exit /b 1
  ...
exit /b 0
```

================
Comment at: llvm/utils/release/build_llvm_release.bat:278
+:run_tests
+  %build_tool% check
+  %build_tool% check-clang
----------------
CarlosAlbertoEnciso wrote:
> hans wrote:
> > 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.
> I have created specific functions for the tests:
> ```
> :: Run 32-bits tests.
> :test_32
>   %build_tool% check || %build_tool% check || %build_tool% check || exit /b 1
>   ...
> exit /b 0
> 
> :: Run 64-bits tests.
> :test_64
>   %build_tool% check || %build_tool% check || %build_tool% check || exit /b 1
>   ...
> exit /b 0
> ```
> 
In relation to the log creation:
- Do you want a command line option?
- Any specific name?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127938