Page MenuHomePhabricator

Update Windows packaging script.
Needs ReviewPublic

Authored by CarlosAlbertoEnciso on Jun 15 2022, 10:50 PM.

Details

Summary

Update the Windows packaging script.

As discussed here:
https://discourse.llvm.org/t/build-llvm-release-bat-script-options/63146/6

These are the changes in this patch:

  • Add command line options to select specific tasks:
    • Specify output directory.
    • Build 32 and/or 64 binaries.
    • Run the tests
    • Package the binaries.
  • In stage-1 use lld-link.
  • Split the build/test/package into separate functions.
  • Added some trace statements.
  • Include llvm-lib and llvm-windres in stage1.

The added command line options are:

--version [<number>]  Version number (i.e. 14.0.4)
--base-dir <name>     Build base directory
--log-file <name>     Log filename

--build-32            Build 32-bit binaries
--build-64            Build 64-bit binaries
--test-32             Run 32-bits tests
--test-64             Run 64-bits tests

--pack-32             Create 32-bits binary package
--pack-64             Create 64-bits binary package
--repack-32           Repack 32-bits binary package
--repack-64           Repack 64-bits binary package

--trace               Debugging trace

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
CarlosAlbertoEnciso requested review of this revision.Jun 15 2022, 10:50 PM
Herald added a project: Restricted Project. · View Herald Transcript
thieta requested changes to this revision.Jun 15 2022, 11:17 PM

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.

llvm/utils/release/build_llvm_release.bat
31

This can't realistically be anything but ninja right? is there a point of this indirection?

51

I think you can default to a output directory instead of failing.

56

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.

57

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.

265

maybe we should use other LLVM tools here as well? CMAKE_AR=llvm-lib CMAKE_RC=llvm-windres etc.

278

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?

287

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.

310

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.

This revision now requires changes to proceed.Jun 15 2022, 11:17 PM
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Jun 16 2022, 2:47 AM
hans added inline comments.Jun 16 2022, 4:59 AM
llvm/utils/release/build_llvm_release.bat
7

Wait, what? This seems undesirable.

16

Unrelated to this patch, but since we no longer build the clang-format plugin, this SDK is no longer needed.

41

"output" isn't a great variable name. How about something like "build_dir_base"?

57

+1, "version" should be what gets fed to the PACKAGE_VERSION cmake variable, and "revision" should be the git revision.

59

I haven't seen batch syntax like this before. I guess it's new?

125

What's this for?

126

Any reason for reordering the projects here?

128

This is a good idea. We may want to drop the 32-bit packages from releases eventually.

138

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.

157

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.

202

Since it takes quite a while to run, can this repackaging step be put behind a flag?

287

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.

310

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.

llvm/utils/release/build_llvm_release.bat
7

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.

16

Removed that comment.

31

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.

41

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%
51

May be use the user temporal directory as default value.

56

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.

57

I will update it.

59

For what I know it was introduced back in Windows NT.
But if you prefer, i can use the explicit checks:

if not "%version%"==""
125

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.

126

No reason.
Put back the original order.

-DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;lld;compiler-rt;lldb;openmp"
128

In our case we build only 64-bits.

157

Putting it back the re-tries for all build steps and test invocations.

202

Any specific name for the flag?

265

Added those tools.

278

Restored the retry logic.

310

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
310

In relation to the log creation:

  • Do you want a command line option?
  • Any specific name?
thieta added inline comments.Jun 16 2022, 11:46 PM
llvm/utils/release/build_llvm_release.bat
7

symlinks requires admin rights on windows unfortunately. But I have never seen this - so it has to be some place where it conditions the symlinking?

51

yeah that works.

56

sounds good to me.

202

CompressInstaller maybe?

310

LogPath or TestLogPath?

llvm/utils/release/build_llvm_release.bat
7

Removed the administrator check. I am assumming it is my local configuration.

138

Done.

202

I am proposing to use long option names to accomodate the CompressInstaller, build_dir_base and LogPath or TestLogPath cases:

--build-32    --build-64     # build step
--test-32     --test-64      # test step
--pack-32     --pack-64      # package step
--repack-32   --repack-64    # repackagin step

--trace                      # debugging trace
--base-dir                   # build directory base
--log-file                   # log filename (in the same build directory)
llvm/utils/release/build_llvm_release.bat
202

Usage test:

Script for building the LLVM installer on Windows, used for
the releases at https://github.com/llvm/llvm-project/releases

Usage: build_llvm_release.bat <options>

options:
--version [<number>]  Version number (i.e. 14.0.4)
--base-dir <name>     Build base directory
--log-file <name>     Log filename

--build-32            Build 32-bit binaries
--build-64            Build 64-bit binaries
--test-32             Run 32-bits tests
--test-64             Run 64-bits tests

--pack-32             Create 32-bits binary package
--pack-64             Create 64-bits binary package
--repack-32           Repack 32-bits binary package
--repack-64           Repack 64-bits binary package

--trace               Debugging trace

Update patch to address the feedback received.

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Jun 20 2022, 1:34 AM
hans added a comment.Jun 20 2022, 2:19 AM

Taking a step back, at this point the patch is almost a rewrite of the current script. It's doing a lot of different things, and I'm not sure I agree with all of them. Perhaps we need to discuss a bit more what are the goals of these changes, and whether they can be done as a series of incremental patches rather than a rewrite.

llvm/utils/release/build_llvm_release.bat
7

Some searching suggests this may be to due to 7zip's behaviour having changed in recent versions. I'm using 18.05 which is probably old by now, but doesn't seem to have this problem.

51

Using the current working directory seems like the more natural choice.

65

"location" isn't very descriptive, how about "source_location" or "source_url"?

72

This doesn't seem right. The version number should match the checked in version number. If that's not possible, perhaps it should be passed as a flag.

125

I don't think the current amount of information is too noisy.

Taking a step back, at this point the patch is almost a rewrite of the current script. It's doing a lot of different things, and I'm not sure I agree with all of them. Perhaps we need to discuss a bit more what are the goals of these changes, and whether they can be done as a series of incremental patches rather than a rewrite.

I am having the same feeling. I think most of the "new things" are useful but may be incremental pacthes. I would suggest to define what we can add on each patch.

llvm/utils/release/build_llvm_release.bat
7

I am using 7-Zip 21.06 (x64).

llvm/utils/release/build_llvm_release.bat
51

Changed to use current directory as default.

65

Changed to "source_location".

125

Removed -DLLVM_LIT_ARGS="-s"

CarlosAlbertoEnciso added a comment.EditedJun 21 2022, 12:37 AM

What I would suggest is to have the following incremental patches:

1) Use the existing script but restoring the deleted packaging step.
   Including a solution for the admin permissions.
2) Refactor the build and test steps into functions.
   Using current directory as output location. No additional command lines.
3) Add cmdline options to select the build, test and package steps.
4) Add cmdline options to select output directory, log filename.
5) Add cmdline options to extend the input branch to include any random branch/sha/tag.
hans added a comment.Jun 21 2022, 5:35 AM
  1. Use the existing script but restoring the deleted packaging step.

Do you mean the repackaging? If so, I'd prefer that to be behind a flag, which means we'd need flag processing first.

Including a solution for the admin permissions.

I've added a comment about that above.

  1. Refactor the build and test steps into functions. Using current directory as output location. No additional command lines.
  2. Add cmdline options to select the build, test and package steps.
  3. Add cmdline options to select output directory, log filename.
  4. Add cmdline options to extend the input branch to include any random branch/sha/tag.
llvm/utils/release/build_llvm_release.bat
7

The behaviour that changed is that older 7zip seems to simply not handle symlinks in zip archives (at least on Windows). If there's a symlink foo -> bar in the archive, it will just create a file "foo" with the contents "bar". And it seems the symlinks were not important enough to break the tests.

I haven't bisected the exact 7zip version where this changed.

These are the symlinks currently in the repository:

$ wget https://github.com/llvm/llvm-project/archive/refs/heads/main.zip
$ zipinfo main.zip | grep '^l'
lrwxrwxrwx  2.3 unx       24 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/CUDA-symlinks/usr/bin/ptxas
lrwxrwxrwx  2.3 unx       30 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/basic_cross_linux_tree/usr/bin/i386-unknown-linux-gnu-ld
lrwxrwxrwx  2.3 unx       32 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/basic_cross_linux_tree/usr/bin/x86_64-unknown-linux-gnu-ld
lrwxrwxrwx  2.3 unx        7 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/basic_cross_linux_tree/usr/i386-unknown-linux-gnu/bin/ld
lrwxrwxrwx  2.3 unx        7 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/basic_cross_linux_tree/usr/x86_64-unknown-linux-gnu/bin/ld
lrwxrwxrwx  2.3 unx       25 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/multilib_32bit_linux_tree/usr/bin/as
lrwxrwxrwx  2.3 unx       25 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/multilib_32bit_linux_tree/usr/bin/ld
lrwxrwxrwx  2.3 unx       35 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/multilib_32bit_linux_tree/usr/i386-unknown-linux/bin/as
lrwxrwxrwx  2.3 unx       35 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/multilib_32bit_linux_tree/usr/i386-unknown-linux/bin/ld
lrwxrwxrwx  2.3 unx       27 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/multilib_64bit_linux_tree/usr/bin/as
lrwxrwxrwx  2.3 unx       27 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/multilib_64bit_linux_tree/usr/bin/ld
lrwxrwxrwx  2.3 unx       37 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/multilib_64bit_linux_tree/usr/x86_64-unknown-linux/bin/as
lrwxrwxrwx  2.3 unx       37 tx stor 22-Jun-21 14:24 llvm-project-main/clang/test/Driver/Inputs/multilib_64bit_linux_tree/usr/x86_64-unknown-linux/bin/ld
lrwxrwxrwx  2.3 unx       13 tx stor 22-Jun-21 14:24 llvm-project-main/libclc/amdgcn-mesa3d
lrwxrwxrwx  2.3 unx        5 tx stor 22-Jun-21 14:24 llvm-project-main/libclc/clspv64
lrwxrwxrwx  2.3 unx       22 tx stor 22-Jun-21 14:24 llvm-project-main/libcxx/test/std/pstl
lrwxrwxrwx  2.3 unx       20 tx stor 22-Jun-21 14:24 llvm-project-main/openmp/tools/analyzer/llvm-openmp-analyzer++

Perhaps we should ask users of this script to use the old 7zip version, or point out that users of newer versions will need to run as administrator? Perhaps only the 7zip part could be run as admin?

llvm/utils/release/build_llvm_release.bat
7

In relation to the 7zip versions with the symbolic links issue:
19.00 works
20.02 works
21.07 Fails
22.00 Fails

Perhaps this should be the first patch and decide on:

  1. Check the 7zip version and ask the user to use an older version or run the script as administrator

or

  1. Run the 7zip part as administrator.
hans added inline comments.Jun 27 2022, 8:12 AM
llvm/utils/release/build_llvm_release.bat
7

Thanks for investigating. We already require pretty specific versions of tools in this script, so maybe we could suggest a specific version of 7zip (and tell people to run the script as admin otherwise).

I have created https://reviews.llvm.org/D129263
That patch address only the symbolic link creation issue. (Administrator permissions and/or 7-zip version).

dyung added a subscriber: dyung.Tue, Jul 12, 12:01 AM

Just a general comment, but when writing batch scripts, I tend to avoid using parenthesis with if blocks like you are proposing here. I've found that in the past they can lead to some obscure issues that I think were impossible to work around if something either in the same line or contained within it expanded to include parenthesis (such as "Program Files (x86)".

So instead of something like this:

if not defined base-dir (
  set "base-dir=%TMP%"
)

I would write it using a goto like this:

if defined base-dir goto :BaseDirDefined
  set "base-dir=%TMP%"
:BaseDirDefined

This way you avoid using potentially problematic parenthesis.

dyung added inline comments.Tue, Jul 12, 12:29 AM
llvm/utils/release/build_llvm_release.bat
2

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.

52

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.

58

Is this missing the return value you intend to exit with?

62

I recommend always enclosing variables that could contain a path in double quotes to deal with possible spaces or parenthesis in the path.

68

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.

178

Will this handle the case where %build_dir% contains a space or parenthesis?

182

For consistency, is there a reason why CMAKE_AR and CMAKE_RC do not specify the tool with .exe at the end?

372

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).