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

Allow install_sct to be run standalone (without downloading "Source code" archive) #4049

Merged
merged 5 commits into from Feb 28, 2023

Conversation

joshuacwnewton
Copy link
Member

Description

Currently, for Linux/macOS installations of SCT, users must navigate to the Release page, then download a "Source code (.zip)" archive. They must then cd into the folder and run install_sct.

This is a bit unwieldy, and has also prevented us from collecting download statistics for the past several years. Without download statistics, it's more difficult to demonstrate the use and impact of SCT (for the purposes of e.g. grant proposals).

As a test, the Windows installer (install_sct.bat) was designed to be run standalone. This allowed us to upload the install script to the release as an asset, which does grant us download statistics.

As a followup, then, this PR updates install_sct so that the Linux/macOS installers so that they can be run standalone, and thus uploaded to releases to benefit from the same statistics gathering.

Future work

  • Add steps to our CI to upload install_sct as a release asset (changing the default git_ref value from master to <release milestone name>)
    • We could potentially even upload install_sct twice as two separate release artifacts, Linux and macOS, to collect better data on who uses which OS.
      (This would also make the installation process for each OS a bit more unified, since install_sct.bat gets renamed to install_sct-5.8_win.bat. Similarly, install_sct could become install_sct-5.8_linux.sh and install_sct-5.8_macos.sh.)
      (This could also lead us down a path where we remove all of the OS-specific if/else blocks that pollute install_sct, since we would maintain dedicated installers for Linux vs. macOS.)
    • This could be possibly be done as part of this PR, too?
  • Update the documentation to describe the new installation process (and clarify that git is now a dependency).
  • Better unify the installation process for Linux/macOS and Windows
    • Windows: Using a more sensible installation directory by default (~/sct_<version> instead of ~/spinalcordtoolbox)
    • Windows: Allowing users to choose a custom installation directory.
    • Windows: Git cloning into a temporary directory first, then copying the files over to the target installation directory.
    • etc.

Linked issues

Fixes #3545.

@joshuacwnewton joshuacwnewton self-assigned this Feb 22, 2023
@joshuacwnewton joshuacwnewton added the installation category: install_sct or pip/setup.py label Feb 22, 2023
@joshuacwnewton joshuacwnewton added this to the 6.0 milestone Feb 22, 2023
There are 3 possibilities for how `install_sct` has been run:
  1. git clone --> ./install_sct
  2. Download "Source code (.zip)" --> ./install_sct
  3. Download install_sct by itself --> ./install_sct

We want to only run `git clone` ourselves in case 3. So, how do we tell
if we already have the source files in the PWD?

ANS: We can re-use the existing check for the version.txt file. :)

This isn't 100% reliable, but it's hard to imagine a situation where
'spinalcordtoolbox/version.txt' isn't present but the rest of the source
isn't. Plus, this minimizes the PR diff.
When `install_sct` is run on its own, it can *never* be an in-place
installation, because we don't have access to the source files in the
PWD. Because of this, we must force SCT_INSTALL_TYPE=package. So, we
move the setting of SCT_INSTALL_TYPE into the earlier if/else block.
@joshuacwnewton joshuacwnewton force-pushed the jn/3545-add-git-clone-to-install_sct branch from 55cf487 to 144c0f9 Compare February 22, 2023 16:25
@mguaypaq mguaypaq self-requested a review February 23, 2023 19:54
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

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

A couple of comments for right now (I'll finish this review a bit later).

install_sct Outdated Show resolved Hide resolved
install_sct Show resolved Hide resolved
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

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

I guess my comments were more of a "request changes" nature. But overall, this looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installation category: install_sct or pip/setup.py
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SCT's current distribution method prevents us from checking download statistics
2 participants