Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Improve default initialization of Slicer_CPACK_NSIS_INSTALL_ROOT #1155

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Jun 19, 2019

If Slicer_CPACK_NSIS_INSTALL_REQUIRES_ADMIN_ACCOUNT is ON, the variable
Slicer_CPACK_NSIS_INSTALL_ROOT is initialized with "$PROGRAMFILES64 or $PROGRAMFILES
as it was done before r28293 (ENH: Allow Slicer install on Windows for non-admin users)

If Slicer_CPACK_NSIS_INSTALL_REQUIRES_ADMIN_ACCOUNT is ON, the variable
Slicer_CPACK_NSIS_INSTALL_ROOT is initialized with "$PROGRAMFILES64 or $PROGRAMFILES
as it was done before r28293 (ENH: Allow Slicer install on Windows for non-admin users)
@jcfr
Copy link
Member Author

jcfr commented Jun 19, 2019

Closing without integrating, requiring custom application to explicitly set Slicer_CPACK_NSIS_INSTALL_REQUIRES_ADMIN_ACCOUNT and Slicer_CPACK_NSIS_INSTALL_ROOT is reasonable.

@lassoan Let me know if you think otherwise

@jcfr jcfr closed this Jun 19, 2019
@jcfr jcfr deleted the improve-Slicer_CPACK_NSIS_INSTALL_ROOT-default-init branch June 19, 2019 14:35
@jcfr
Copy link
Member Author

jcfr commented Jun 19, 2019

if(NOT DEFINED Slicer_CPACK_NSIS_INSTALL_ROOT)
# Set root install directory (displayed to end user at installer-run time)
# to C:\Users\<username>\AppData\Local by default to allow installation
# without having administrative privileges.
set(Slicer_CPACK_NSIS_INSTALL_ROOT "$LOCALAPPDATA\\\\${Slicer_ORGANIZATION_NAME}" CACHE STRING
set(_install_root_default "$LOCALAPPDATA\\\\${Slicer_ORGANIZATION_NAME}")
Copy link
Contributor

@lassoan lassoan Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. It would be a bit more readable like this:

 # Set default install directory (displayed to end user during installation)
 if(Slicer_CPACK_NSIS_INSTALL_REQUIRES_ADMIN_ACCOUNT)
  # User has administrative privileges, therefore we can install to shared folder
  # "C:\Program Files" or "c:\Program Files (x86)".
  if(CMAKE_CL_64)
    set(_install_root_default "$PROGRAMFILES64")
  else()
    set(_install_root_default "$PROGRAMFILES")
  endif()
else()
  # We do not require administrative privileges, therefore we install to user folder
  # "C:\Users\<username>\AppData\Local".
  set(_install_root_default "$LOCALAPPDATA\\\\${Slicer_ORGANIZATION_NAME}")
endif()

@jcfr
Copy link
Member Author

jcfr commented Jun 19, 2019

Thanks for the feedback. I ended up incorporating your suggestion and integrating as r28313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants