Initial ISIS install script by acpaquette · Pull Request #5741 · DOI-USGS/ISIS3

acpaquette · 2025-03-06T22:40:41Z

Description

Initial ISIS install script for users, based off of an initial version from @Kelvinrr

Related Issue

Docs IAA

How Has This Been Validated?

User testing

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Documentation change (update to the documentation; no code change)
Breaking change (fix or feature that would cause existing functionality to change)
Infrastructure change (changes to things like CI or the build system that do not impact users)

Checklist:

I have read and agree to abide by the Code of Conduct
I have read the CONTRIBUTING document.
My change requires a change to the documentation and I have updated the documentation accordingly.
I have added tests to cover my changes.
I have added myself to the .zenodo.json document.
I have added my user impacting change to the CHANGELOG.md document.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

github-actions · 2025-03-06T22:41:32Z

The build and test suite have started for your pull request.

To view your build log, please reference the build with source version: "PR_5741".

Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch.

dpmayerUSGS

Some genuine bugs in the argument parser, but otherwise mostly stylistic/convention considerations.

isis/scripts/install_isis.sh

Kelvinrr · 2025-03-20T16:41:33Z

isis/scripts/install_isis.sh

+    printf "\t-v, --isis_version    Different ISIS versions as defined by the anaconda versions "
+    printf "for a label at https://anaconda.org/usgs-astrogeology/isis, examples include 8.0.3 "
+    printf "(LTS/Feature), 8.2.0_RC1 (RC), and 2025.02.22 (dev)\n "
+    printf "\t-n, --env_name        The name of the anaconda environment to create.\n"


I get unknown option:

(isisdevinstall) igswza281l2305:~ krodriguez$ bash install_isis.sh -l dev -n isisbnash Unknown option -n

github-actions · 2025-03-31T22:04:56Z

The build and test suite have started for your pull request.

To view your build log, please reference the build with source version: "PR_5741".

Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch.

Kelvinrr · 2025-03-31T22:21:41Z

@dpmayerUSGS fixed a few bugs, mind re-reviewing this?

dpmayerUSGS

I only reviewed isis/scripts/install_isis.sh.

I think the script could be slimmed down. There appear to be some unnecessary code blocks.

One of the user prompts says that mamba is recommended , but the script will exit with an error if the user has conda installed but declines to install mamba. What is the intended behavior?

dpmayerUSGS · 2025-03-31T23:58:12Z

isis/scripts/install_isis.sh

+
+read -r ans
+
+# If no input, use default path


This if block seems unnecessary because the script defines ans="no" as default a few lines above. I think it's okay to keep it if you want to be "belt and suspenders," but maybe update the comment text; the block doesn't have anything to do with a "default path."

I don't think this is true, since user input will overwrite that default to whatever was put in. We can remove it and check for empty string later down the line. Otherwise, if they just press enter, ans will be an empty string.

This applies to all the similar comments.

Okay, I see what you mean now.
The repeated handling of yes/no responses could probably be abstracted away into a function that is called where needed rather than repeating the same if block though. I would tighten the code up a bit.

dpmayerUSGS · 2025-03-31T23:58:27Z

isis/scripts/install_isis.sh

+
+    read -r ans
+
+    # If no input, use default path


This if block seems unnecessary because the script defines ans="no" as default a few lines above. I think it's okay to keep it if you wantto be "belt and suspenders," but maybe update the comment text; the block doesn't have anything to do with a "default path."

dpmayerUSGS · 2025-04-01T00:05:25Z

isis/scripts/install_isis.sh

+    $MINIFORGE_DIR/bin/$CLIENT init || exit 1
+else
+    echo "Miniforge is already installed."
+    if [[ "$CONDA_PREFIX" == *"envs"* ]]; then


Is this trying to look for a directory named /envs/ somewhere in $CONDA_PREFIX? If yes, consider changing the pattern to *"/envs/"* to the pattern more specific.

dpmayerUSGS · 2025-04-01T15:50:50Z

isis/scripts/install_isis.sh

+
+$MINIFORGE_DIR/bin/$CLIENT env config vars set -n $ENV_NAME ISISDATA=$ISISDATA_PREFIX ISISROOT=$MINIFORGE_DIR/envs/$ENV_NAME || failed_command "Mamba config var set"
+
+if [[ "$MINIFORGE_DIR" == *"envs/$ENV_NAME"* ]]; then


Consider inserting / before envs to make the pattern more specific.

dpmayerUSGS · 2025-04-01T16:07:01Z

isis/scripts/install_isis.sh

+    read -r ans
+
+    # If no input, use default
+    if [ "$ans" = "" ]; then


~~This if block seems unnecessary because the script defines ans="no" as default a few lines above.~~

dpmayerUSGS · 2025-04-01T16:09:01Z

isis/scripts/install_isis.sh

+    fi
+
+    ans=$(echo "${ans}" | tr '[:lower:]' '[:upper:]')
+    while [ "$ans" != "YES" ] && [ "$ans" != "NO" ]


This while seems unnecessary because the script defines ans=no by default above.

similar to my previous comment, user input overwrites that value and we need a loop to make sure they didn't input something ambiguous

dpmayerUSGS · 2025-04-01T16:11:32Z

isis/scripts/install_isis.sh

+# Check if conda is installed but mamba is not
+if command -v conda &> /dev/null && ! command -v mamba &> /dev/null; then
+    echo "WARNING: conda is installed but mamba is not."
+    echo "mamba is a faster alternative to conda and is recommended for ISIS installation."


The word "recommended" seams to weak here. If the user passes "no" in response to the prompt, the if block on Line 221 prints a message that only mamba is supported and the script exits.

The prompt should be clear that mamba is actually required or the script should be modified to proceed with an existing install of conda without mamba.

dpmayerUSGS · 2025-04-01T16:15:10Z

isis/scripts/install_isis.sh

+fi
+
+ans=$(echo "${ans}" | tr '[:lower:]' '[:upper:]')
+while [ "$ans" != "YES" ] && [ "$ans" != "NO" ]


This while seems unnecessary because the script defines ans=no by default above.

dpmayerUSGS · 2025-04-01T16:15:22Z

isis/scripts/install_isis.sh

+    fi
+
+    ans=$(echo "${ans}" | tr '[:lower:]' '[:upper:]')
+    while [ "$ans" != "YES" ] && [ "$ans" != "NO" ]


~~This while seems unnecessary because the script defines ans=no by default above.~~

Kelvinrr · 2025-04-01T21:15:09Z

Addressed comments, seems this needs to be rebased which I can do after these comments are resolved.

github-actions · 2025-04-01T21:15:31Z

The build and test suite have started for your pull request.

To view your build log, please reference the build with source version: "PR_5741".

Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch.

dpmayerUSGS

Only reviewed isis/scripts/install_isis.sh
Changes look good.

github-actions · 2025-04-02T19:17:10Z

The build and test suite have started for your pull request.

To view your build log, please reference the build with source version: "PR_5741".

Additionally, check the latest "dev" source version to identify existing test failures. Please note that you are not responsible for the test failures that exist on both your PR and the dev branch.

dpmayerUSGS suggested changes Mar 7, 2025

View reviewed changes

Kelvinrr reviewed Mar 11, 2025

View reviewed changes

isis/scripts/install_isis.sh Show resolved Hide resolved

isis/scripts/install_isis.sh Outdated Show resolved Hide resolved

Kelvinrr reviewed Mar 24, 2025

View reviewed changes

Kelvinrr requested a review from dpmayerUSGS March 31, 2025 22:21

dpmayerUSGS suggested changes Apr 1, 2025

View reviewed changes

Kelvinrr requested a review from dpmayerUSGS April 1, 2025 21:30

dpmayerUSGS reviewed Apr 1, 2025

View reviewed changes

force rebase of initial isis script

0fcd488

Kelvinrr force-pushed the isis_install branch from 0d75eb0 to 0fcd488 Compare April 2, 2025 19:16

Kelvinrr approved these changes Apr 2, 2025

View reviewed changes

Kelvinrr merged commit cd7a9ea into DOI-USGS:dev Apr 2, 2025
2 checks passed


		$MINIFORGE_DIR/bin/$CLIENT env config vars set -n $ENV_NAME ISISDATA=$ISISDATA_PREFIX ISISROOT=$MINIFORGE_DIR/envs/$ENV_NAME \|\| failed_command "Mamba config var set"

		if [[ "$MINIFORGE_DIR" == "envs/$ENV_NAME" ]]; then

Conversation

acpaquette commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

How Has This Been Validated?

Types of changes

Checklist:

Licensing

Uh oh!

github-actions bot commented Mar 6, 2025

Uh oh!

dpmayerUSGS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 31, 2025

Uh oh!

Kelvinrr commented Mar 31, 2025

Uh oh!

dpmayerUSGS left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kelvinrr Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpmayerUSGS Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpmayerUSGS Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpmayerUSGS Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kelvinrr commented Apr 1, 2025

Uh oh!

github-actions bot commented Apr 1, 2025

Uh oh!

dpmayerUSGS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Apr 2, 2025

Uh oh!

Reviewers

Assignees

Labels

acpaquette commented Mar 6, 2025 •

edited

Loading

dpmayerUSGS left a comment •

edited

Loading

Kelvinrr Apr 1, 2025 •

edited

Loading

dpmayerUSGS Mar 31, 2025 •

edited

Loading

dpmayerUSGS Apr 1, 2025 •

edited

Loading

dpmayerUSGS Apr 1, 2025 •

edited

Loading