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

DM-3372: Provenance support #5

Merged
merged 6 commits into from Jun 8, 2016
Merged

DM-3372: Provenance support #5

merged 6 commits into from Jun 8, 2016

Conversation

timj
Copy link
Member

@timj timj commented May 23, 2016

No description provided.

return version

def getPythonPackages():
"""Provide a dict of imported python packages and their versions
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First time I read this I thought it was telling me that I was supposed to give a dict as an argument to the function. I think using "Provides" rather than "Provide" would avoid that confusion, or alternatively "Return" if the tense is a problem..

@PaulPrice PaulPrice force-pushed the tickets/DM-3372 branch 3 times, most recently from 80596ef to 1fe6f69 Compare June 2, 2016 19:13
# we don't tend to run "scons" every time we update some python file, and even if we did sconsUtils
# probably doesn't check to see if the repo is clean).
for prod in products:
if not prod.version.startswith(Product.Product.LocalVersionPrefix):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are too many Products in there. Breaks on my laptop unless I remove one of them and use Product.LocalVersionPrefix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmph. It broke on Jenkins without both of them. I'll see if I can find a middle road.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>>> from eups import Product
>>> Product.LocalVersionPrefix
'LOCAL:'

@PaulPrice
Copy link
Contributor

OK, give that a whirl.

@timj
Copy link
Member Author

timj commented Jun 3, 2016

Everything now builds on my Mac so that's good. I'm somewhat concerned that there are no tests for this functionality in base itself and that it took until pipe_base before we found a basic problem. Would it be possible for some basic tests to exist in base even if that means working out the version of base, sconsUtils and boost (am I correct in thinking that boost is no longer a dependency of base? It says it is but I think the recent changes mean it isn't) just to at least prove that a version can be found (even if you aren't checking what the value is).

This attempts to provide a platform-independent interface to dlopen
and dlsym.
We get the versions by loading a dynamic library and pulling out the
appropriate symbol.  Includes support for getting the versions of
cfitsio, fftw, wcslib, gsl.
if __name__ == "__main__":
run(True)
sys.exit(unittest.TextTestRunner().run(suite()).wasSuccessful())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this all be unittest.main()? suites are deprecated.

@PaulPrice
Copy link
Contributor

Here's a fixup to address the latest comments.

self.assertDictEqual(new.difference(packages), {})
self.assertDictEqual(new.missing(packages), {}) # Nothing in 'new' that's not in 'packages'
extra = new.extra(packages)
self.assertNotEqual(len(extra), 0) # 'new' has extra stuff compared to 'packages'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertGreater as well I guess.

@timj
Copy link
Member Author

timj commented Jun 7, 2016

The tests now pass on my Mac with unittest.main().

@PaulPrice
Copy link
Contributor

Ready to merge (after rebasing)?

@timj
Copy link
Member Author

timj commented Jun 7, 2016

Probably. I'm actually waiting for my build to get as far as pipe_base...

@PaulPrice
Copy link
Contributor

OK. I just fired off Jenkins again, just in case. "What, race?"

By iterating with sys.modules.iteritems(), we're subject to a race
condition: if there's another thread (e.g., in scons) that does an
import, sys.modules will change while we're iterating over it.
Unfortunately, we're somewhat limited in what we can test because we
only get the versions of things being used at runtime, and this
package sits rather low in the dependency chain so there's not usually
a lot of other packages available when this test gets run.  Therefore
some of the tests are only checking that things don't explode, since
they are incapable of testing much more than that. But better that
than nothing.
@PaulPrice PaulPrice merged commit 5b85112 into master Jun 8, 2016
@ktlim ktlim deleted the tickets/DM-3372 branch August 25, 2018 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants