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

Make singularity-wrapper more portable #82

Merged

Conversation

kjsanger
Copy link
Member

This replaces use of bash in the wrapper generator that is hosted inside the container, making it usable in containers that do not have bash installed e.g. Alpine Linux.

It also removes a dependency on jq, again for portability. This requires a change the manifest of executables from JSON to a flat list.

This replaces use of bash in the wrapper generator that is hosted
inside the container, making it usable in containers that do not have
bash installed e.g. Alpine Linux.

It also removes a dependency on jq, again for portability. This
requires a change the manifest of executables from JSON to a flat
list.
@kjsanger kjsanger added the enhancement New feature or request label Oct 11, 2023
@dkj
Copy link
Member

dkj commented Oct 16, 2023

Don't know how important these are. Shellcheck reports:


Line 79:
    local dir="$1"
    ^-- SC3043 (warning): In POSIX sh, 'local' is undefined.
 
Line 80:
    local exe="$2"
    ^-- SC3043 (warning): In POSIX sh, 'local' is undefined.
 
Line 84:
        echo -e "\nERROR:\n  A Docker image name is required"
             ^-- SC3037 (warning): In POSIX sh, echo flags are undefined.
 
Line 110:
    local dir="$PREFIX/bin"
    ^-- SC3043 (warning): In POSIX sh, 'local' is undefined.
 
Line 164:
            echo -e "\nERROR:\n  Invalid option"
                 ^-- SC3037 (warning): In POSIX sh, echo flags are undefined.
 
Line 173:
    echo -e "\nERROR:\n  The manifest of executables at '$MANIFEST_PATH' does not exist"
         ^-- SC3037 (warning): In POSIX sh, echo flags are undefined.
 
Line 177:
operation="$@"
          ^-- SC2124 (warning): Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
 
Line 180:
    echo -e "\nERROR:\n  An operation argument is required"
         ^-- SC3037 (warning): In POSIX sh, echo flags are undefined.
 
Line 194:
        echo -e "\nERROR:\n  Invalid wrapper operation '$operation'"
             ^-- SC3037 (warning): In POSIX sh, echo flags are undefined.

(Assign as array was already there).

Are the local and echo -e significant?

@kjsanger
Copy link
Member Author

Are the local and echo -e significant?

They are supported by dash, which is the default shell for alpine and busybox, which are the main alternatives to Debian/Redhat (and derivatives) that we're likely to see.

The array assignment is wrong (but accidentally give the correct result here, as it turns out). It should be just operation="$1" because the script arguments have been shifted relative to OPTIND a few lines previously. I will fix that.

@jmtcsngr jmtcsngr merged commit d9248c5 into wtsi-npg:devel Oct 26, 2023
@kjsanger kjsanger deleted the feature/portable-singularity-wrapper branch October 27, 2023 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants