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

    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

            Activity

            Hide
            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
            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
            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
            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
            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
            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
            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:

                  Summary Panel