This is an archive of the discontinued LLVM Phabricator instance.

Windows packaging script. Check administrator permissions and/or 7-Zip version.
ClosedPublic

Authored by CarlosAlbertoEnciso on Jul 7 2022, 1:38 AM.

Details

Summary

Update the Windows packaging script.

As discussed here:
https://discourse.llvm.org/t/build-llvm-release-bat-script-options/63146/6
https://reviews.llvm.org/D127938#inline-1234370

Latest 7-zip versions (21.x) require administrator permissions to create symbolic links.

These are the changes in this patch:

  • Check if any of the following conditions is true: a) Version of 7-zip is 20.x or older b) Script is running with administrator permissions

Diff Detail

Event Timeline

CarlosAlbertoEnciso requested review of this revision.Jul 7 2022, 1:38 AM
CarlosAlbertoEnciso created this revision.
hans added inline comments.Jul 7 2022, 4:41 AM
llvm/utils/release/build_llvm_release.bat
4

This line is saying the same as line 26. If we move the 7zip version check down before :begin, there would be no need to repeat it, and I think it would make sense to do that anyway.

6

I'd suggest baking these notes into the one right before the actual check. Also a little more context may be needed -- the reader might not know why it's desirable to create symlinks.

How about something like : "7zip versions 21.x and higher will try to extract the symlinks in llvm's git archive, which requires running as administrator."

12

Any reason not to do 1-9 instead of 1-3?

llvm/utils/release/build_llvm_release.bat
4

Good point.

6

All notes replaced with

REM Note:
REM   7zip versions 21.x and higher will try to extract the symlinks in
REM   llvm's git archive, which requires running as administrator.

REM Check for correct 7-zip version and/or administrator permissions.
....
12

Changing to 1-9.

Address feedback from @hans

  • Better description for the reason to create symlinks.
  • Expand regular expression to include versions 21 up to 29.
thieta accepted this revision.Jul 8 2022, 1:42 AM

LGTM - maybe a small comment, but won't block on that.

llvm/utils/release/build_llvm_release.bat
24

Took me a while to understand this check - I am guessing you would only have permissions to see it if you are admin? Maybe that should be a comment to explain. There is no better way to test for admin rights in bat?

This revision is now accepted and ready to land.Jul 8 2022, 1:42 AM
hans accepted this revision.Jul 8 2022, 2:13 AM

lgtm, thanks!

llvm/utils/release/build_llvm_release.bat
21

Nit: maybe drop "correct" since they're all correct. How about just "Check 7-zip version and/or administrator permissions."

llvm/utils/release/build_llvm_release.bat
24

Another way to test for administrator rights is using the mklink command. From my understanding, it requires elevated permissions:

mklink /D link target

Thanks to @hans and @thieta for the patch approval.

But looking with more detail, I found an issue while determining if the script is running with elevated permissions.

  • The test is expecting that the %SYSTEMROOT%\SYSTEM32\WDI\LOGFILES permissions have not been changed.

However, those permissions can be changed (at least on my machine as normal user), following the following steps:

  1. Try to open that folder with Windows Explorer.
  2. You get a warning message You don't have currently permission to acces this folder.
  3. An option is presented Click Continue to permanently have access to this folder.
  4. After that the script will fail, as the user now has access to that folder.

I found another way (I think more robust) to check for administrator, by using the 'mklink' command.

  • It requires administrator permissions to create a symbol link.

Uploaded new patch with these changes.

Change the test for administrator permissions by using the mklink command.

hans accepted this revision.Jul 11 2022, 3:35 AM

still lgtm (just a few nits)

llvm/utils/release/build_llvm_release.bat
23

I think you mean "temporary" instead of "temporal", here and below.

24

I'd suggest moving this line to where link_name is first used below.

This revision was landed with ongoing or failed builds.Jul 11 2022, 10:39 PM
This revision was automatically updated to reflect the committed changes.
llvm/utils/release/build_llvm_release.bat
21

Replaced.

23

Changed to temporary.

24

Moved link_name inside the if.