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

Provide a function to return the path to a package, given its name

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: utils
    • Story Points:
      2
    • Sprint:
      Science Pipelines DM-S15-2
    • Team:
      Alert Production

      Description

      As per RFC-44 we want a simple function in utils that returns the path to a package given a package name. This has the same API as eups.getProductDir, but hides our dependence on eups, as per the RFC.

        Attachments

          Issue Links

          blocks

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-2774 Remove eups dependency from tests

          • Invalid

          Improvement - An improvement or enhancement to an existing feature or task. DM-2636 Update code to use the function provided in DM-2635

          • Done
          is blocked by

          RFC - An issue whose primary purpose is to advertise an intent and allow stakeholders to comment. RFC-47 Remove unused utils functions and change eups::productDir to getPackageDir

          • Implemented
          relates to

          Bug - A problem which impairs or prevents the functions of the product. DM-2776 Remove version functions that rely on SVN

          • Done

          RFC - An issue whose primary purpose is to advertise an intent and allow stakeholders to comment. RFC-44 Remove build system dependencies from tests

          • Implemented

            Activity

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            rowen Russell Owen added a comment - - edited

            I added getPackageDir to utils on tickets/DM-2635 and included a simple unit test.

            I chose to implement the function using environment variables, rather than using the productDir method that sconsUtils adds to eups. If standard eups included productDir as a method, or the function was clearly part of sconsUtils instead of having sconsUtils add it to eups, I would have chosen differently, because it makes me nervous to have two different implementations. But at least the implementation is trivial and the sconsUtils version will only be used inside of sconsUtils once DM-2636 is implemented.

            Show
            rowen Russell Owen added a comment - - edited I added getPackageDir to utils on tickets/ DM-2635 and included a simple unit test. I chose to implement the function using environment variables, rather than using the productDir method that sconsUtils adds to eups. If standard eups included productDir as a method, or the function was clearly part of sconsUtils instead of having sconsUtils add it to eups, I would have chosen differently, because it makes me nervous to have two different implementations. But at least the implementation is trivial and the sconsUtils version will only be used inside of sconsUtils once DM-2636 is implemented.
            Hide
            Permalink
            rowen Russell Owen added a comment -

            After putting it up for review I realized there was a C++ wrapper as well. So I modified the code as follows:

            • Rename the C++ wrapper from lsst::utils::eups::productDir to lsst::utils::getPackageDir and removed the extra version argument that was not actually usable. This function throws an exception if the package is not found.
            • Made the Python code a SWIG wrapper of the C++ code. So now getPackageDir does throw an exception, which (having converted all the code in afw) I think is a marked improvement.

            My apologies for the post-review-state changes. However, I think the results are significantly improved, and I've implemented all suggested changes where they still make sense.

            Show
            rowen Russell Owen added a comment - After putting it up for review I realized there was a C++ wrapper as well. So I modified the code as follows: Rename the C++ wrapper from lsst::utils::eups::productDir to lsst::utils::getPackageDir and removed the extra version argument that was not actually usable. This function throws an exception if the package is not found. Made the Python code a SWIG wrapper of the C++ code. So now getPackageDir does throw an exception, which (having converted all the code in afw) I think is a marked improvement. My apologies for the post-review-state changes. However, I think the results are significantly improved, and I've implemented all suggested changes where they still make sense.
            Hide
            Permalink
            tjenness Tim Jenness added a comment -

            Looks okay. How do the AFW patches relate to DM-2636? Do all the changes have to happen at once because the previous EUPS utility function has disappeared?

            Show
            tjenness Tim Jenness added a comment - Looks okay. How do the AFW patches relate to DM-2636 ? Do all the changes have to happen at once because the previous EUPS utility function has disappeared?
            Hide
            Permalink
            rowen Russell Owen added a comment -

            I changed afw on the same ticket because some C++ examples and tests were using lsst::utils::eups::packageDir, which has been replaced by lsst::utils::getPackageDir. While I was at it, I also updated the python code that was using eups.packageDir, though I could have waited until DM-2636 for that. afw is the only package I found that uses lsst::utils::eups::packageDir so I think this ticket is nicely self-contained. I will start a buildbot run shortly to see if I missed anything.

            Show
            rowen Russell Owen added a comment - I changed afw on the same ticket because some C++ examples and tests were using lsst::utils::eups::packageDir, which has been replaced by lsst::utils::getPackageDir. While I was at it, I also updated the python code that was using eups.packageDir, though I could have waited until DM-2636 for that. afw is the only package I found that uses lsst::utils::eups::packageDir so I think this ticket is nicely self-contained. I will start a buildbot run shortly to see if I missed anything.
            Hide
            Permalink
            tjenness Tim Jenness added a comment -

            That's fine. I was a bit worried that splitting this task into 2 tickets would only work if the old interface remained until the second ticket was completed.

            Show
            tjenness Tim Jenness added a comment - That's fine. I was a bit worried that splitting this task into 2 tickets would only work if the old interface remained until the second ticket was completed.
            Hide
            Permalink
            tjenness Tim Jenness added a comment -

            I have just noticed that diffimLib.py in ip_diffim (derived from diffimLib.i attempts to use guessSvnVersion in diffimlib.version. The entire version function seems anomalous. In fact, I'm suspicious of any code that is trying to deal with $HeadURL$ (pex_logging being the other package if you assume that sconsUtils has to have support for Svn).

            Show
            tjenness Tim Jenness added a comment - I have just noticed that diffimLib.py in ip_diffim (derived from diffimLib.i attempts to use guessSvnVersion in diffimlib.version . The entire version function seems anomalous. In fact, I'm suspicious of any code that is trying to deal with $HeadURL$ (pex_logging being the other package if you assume that sconsUtils has to have support for Svn).
            Hide
            Permalink
            jbosch Jim Bosch added a comment -

            Yup, that's a relic, and it's safe to just delete it. We now machine-generate a version.py as part of every sconsUtils-based build.

            Show
            jbosch Jim Bosch added a comment - Yup, that's a relic, and it's safe to just delete it. We now machine-generate a version.py as part of every sconsUtils -based build.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Tim Jenness
              Watchers:
              Jim Bosch, Kian-Tat Lim, Robert Lupton, Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.