# 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:
• Labels:
• Story Points:
2
• Sprint:
Science Pipelines DM-S15-2
• Team:

#### 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.

#### Activity

Hide
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
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
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
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
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
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
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
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
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
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
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
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
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
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:
Russell Owen
Reporter:
Russell Owen
Reviewers:
Tim Jenness
Watchers:
Jim Bosch, Kian-Tat Lim, Robert Lupton, Russell Owen, Tim Jenness