[PATCH] D127938: Update Windows packaging script.

[Tobias Hieta via Phabricator via llvm-commits]

thieta 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 (
----------------
CarlosAlbertoEnciso wrote:
> 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.
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?


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


================
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"
----------------
CarlosAlbertoEnciso wrote:
> 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.
sounds good to me.


================
Comment at: llvm/utils/release/build_llvm_release.bat:184
+
+  7z x -y LLVM-%package_version%-win32.exe -orepack
+  rmdir /s /q repack\$PLUGINSDIR
----------------
CarlosAlbertoEnciso wrote:
> 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?
CompressInstaller maybe?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127938