[PATCH] D127938: Update Windows packaging script.

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

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.



================
Comment at: llvm/utils/release/build_llvm_release.bat:65
+
+set location=https://github.com/llvm/llvm-project/archive/refs/heads/main.zip
+
----------------
"location" isn't very descriptive, how about "source_location" or "source_url"?


================
Comment at: llvm/utils/release/build_llvm_release.bat:72
+) else (
+  set "version=99.9.9"
+)
----------------
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.


================
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:
> thieta wrote:
> > 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?
> Removed the administrator check. I am assumming it is my local configuration.
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.


================
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:
> 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.
Using the current working directory seems like the more natural choice.


================
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"
----------------
CarlosAlbertoEnciso wrote:
> 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.
I don't think the current amount of information is too noisy.


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

https://reviews.llvm.org/D127938