Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-14026

newinstall.sh depends on `which` -- an undocumented dependency

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: stack release
    • Labels:
      None

      Description

      Paul Price reported via slack yesterday that which is required to run newinstall.sh on the official docker.io/centos images. Currently, which is not a documented dependency. Additionally, the eups build requires which.

      Paul Price [8:09 PM]
      Just discovered that the `which` package is missing from the list of Centos prereqs (https://pipelines.lsst.io/install/prereqs/centos.html).
       
      (If I Docker `FROM centos:7`, I don't have `which`, but `newinstall.sh` uses it.) (edited)

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            mariadb and doxygen packages also use which in their eupspkg.cfg.sh files.

            Show
            tjenness Tim Jenness added a comment - mariadb and doxygen packages also use which in their eupspkg.cfg.sh files.
            Hide
            tjenness Tim Jenness added a comment -

            I filed an EUPS ticket to remind me. https://github.com/RobertLuptonTheGood/eups/issues/127

            Show
            tjenness Tim Jenness added a comment - I filed an EUPS ticket to remind me.  https://github.com/RobertLuptonTheGood/eups/issues/127
            Hide
            jhoblitt Joshua Hoblitt added a comment - - edited

            Changes covered under this ticket:

            • newinstall.sh no longer uses which directly.
            • The newinstall.sh required commands list has been amended to include which perl sed awk.
            • which (along with sed and awk) has been added to the list of explicit packages installed in the docker base images and the pipeline docs.
            • A travis ci config to run shellcheck was added to the repos in which is was fairly trivial to fix pre-existing warnings.
            • I believe I've found most of the usage of which under the github lsst org and opened PRs with the exception of lsst/starlink_ast (3rd party code) and lsst/lsstserver (appears to have an old copy of newinstall.sh).

            I didn't expect to invest 2+ hours into this already, so I did not check any of the other DM related GH orgs.

            Show
            jhoblitt Joshua Hoblitt added a comment - - edited Changes covered under this ticket: newinstall.sh no longer uses which directly. The newinstall.sh required commands list has been amended to include which perl sed awk . which (along with sed and awk ) has been added to the list of explicit packages installed in the docker base images and the pipeline docs. A travis ci config to run shellcheck was added to the repos in which is was fairly trivial to fix pre-existing warnings. I believe I've found most of the usage of which under the github lsst org and opened PRs with the exception of lsst/starlink_ast (3rd party code) and lsst/lsstserver (appears to have an old copy of newinstall.sh ). I didn't expect to invest 2+ hours into this already, so I did not check any of the other DM related GH orgs.
            Show
            jhoblitt Joshua Hoblitt added a comment - stack-os-matrix build of lsst_distrib : https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/27788/pipeline
            Hide
            price Paul Price added a comment -

            Thanks for jumping on this, and so thoroughly!

            I'm happy with the changes you made to the ci_* packages.

            Show
            price Paul Price added a comment - Thanks for jumping on this, and so thoroughly! I'm happy with the changes you made to the ci_* packages.
            Hide
            tjenness Tim Jenness added a comment -

            I'm happy with the code changes. I'd like to move the repositories that have had no activity in five years to legacy. I'd also like a statement from Kian-Tat Lim concerning the Travis shellcheck scripts that have been added throughout.

            Show
            tjenness Tim Jenness added a comment - I'm happy with the code changes. I'd like to move the repositories that have had no activity in five years to legacy. I'd also like a statement from Kian-Tat Lim concerning the Travis shellcheck scripts that have been added throughout.
            Hide
            ktl Kian-Tat Lim added a comment -

            I agree that https://github.com/lsst-dm/dm_dev_guide/blob/master/stack/adding-a-new-package.rst#configuring-github-repositories needs 1) separate text saying that repos containing sh or bash scripts may use additional Travis checks, 2) a separate example of shellcheck YAML to be included, not an extension of the current example which is explicitly labeled "flake8", and 3) (but not your responsibility) another separate example of tidy YAML to be included.

            In addition, it seems to me that https://github.com/koalaman/shellcheck#travis-ci is saying that the YAML to be included can be simpler, no?

            Show
            ktl Kian-Tat Lim added a comment - I agree that https://github.com/lsst-dm/dm_dev_guide/blob/master/stack/adding-a-new-package.rst#configuring-github-repositories needs 1) separate text saying that repos containing sh or bash scripts may use additional Travis checks, 2) a separate example of shellcheck YAML to be included, not an extension of the current example which is explicitly labeled "flake8", and 3) (but not your responsibility) another separate example of tidy YAML to be included. In addition, it seems to me that https://github.com/koalaman/shellcheck#travis-ci is saying that the YAML to be included can be simpler, no?
            Hide
            jhoblitt Joshua Hoblitt added a comment -

            Re #3 – no, per the example, shellcheck and the official docker image require each script to be evaluated to be passed as an explicit argument.

            docker run -v "$PWD:/mnt" koalaman/shellcheck myscript
            

            Show
            jhoblitt Joshua Hoblitt added a comment - Re #3 – no, per the example, shellcheck and the official docker image require each script to be evaluated to be passed as an explicit argument. docker run -v "$PWD:/mnt" koalaman/shellcheck myscript
            Hide
            jhoblitt Joshua Hoblitt added a comment -

            Also note that the travis example given is for shellcheck being the only configuration run.

            Show
            jhoblitt Joshua Hoblitt added a comment - Also note that the travis example given is for shellcheck being the only configuration run.
            Hide
            jhoblitt Joshua Hoblitt added a comment -

            I have opened DM-14080 and DM-14081 against architecture for the repos Tim Jenness suggested should be deprecated.

            Per Kian-Tat Lim request, I have updated the dev guide PR to have a separate shellcheck example.

            Show
            jhoblitt Joshua Hoblitt added a comment - I have opened DM-14080 and DM-14081 against architecture for the repos Tim Jenness suggested should be deprecated. Per Kian-Tat Lim request, I have updated the dev guide PR to have a separate shellcheck example.
            Hide
            ktl Kian-Tat Lim added a comment -

            I approve the dev guide change.

            Show
            ktl Kian-Tat Lim added a comment - I approve the dev guide change.
            Hide
            ktl Kian-Tat Lim added a comment -

            Is this still waiting on something?  I'm removing myself from the reviewers list.

            Show
            ktl Kian-Tat Lim added a comment - Is this still waiting on something?  I'm removing myself from the reviewers list.
            Hide
            tjenness Tim Jenness added a comment -

            It seems to be the mariadb and dev guide PRs that are still open. Kian-Tat Lim I think if you approve the dev guide changes we can put the ticket in Reviewed state.

            Show
            tjenness Tim Jenness added a comment - It seems to be the mariadb and dev guide PRs that are still open. Kian-Tat Lim I think if you approve the dev guide changes we can put the ticket in Reviewed state.
            Hide
            jhoblitt Joshua Hoblitt added a comment -

            The dev guide PR have been retracted and no further action items remain on this ticket.

            Show
            jhoblitt Joshua Hoblitt added a comment - The dev guide PR have been retracted and no further action items remain on this ticket.

              People

              • Assignee:
                jhoblitt Joshua Hoblitt
                Reporter:
                jhoblitt Joshua Hoblitt
                Reviewers:
                Jonathan Sick
                Watchers:
                Gabriele Comoretto, Jonathan Sick, Joshua Hoblitt, Kian-Tat Lim, Paul Price, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel