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

Skip SPARQLWrapper test if SPARQLWrapper is not installed #359

Merged
merged 1 commit into from Feb 28, 2014

Conversation

dbs
Copy link
Contributor

@dbs dbs commented Feb 26, 2014

Trying to package RDFLib 4.1.0 on Fedora, I ran into a blocker with this test.
SPARQLWrapper is not yet packaged for Fedora, and unlike other tests which are
skipped if SPARQLWrapper is not installed, this one throws a fatal error. So
let's skip it like the others.

Signed-off-by: Dan Scott dan@coffeecode.net

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 9f7aefa on dbs:skip_sparqlstore into 1282d41 on RDFLib:master.

@joernhees
Copy link
Member

I'm torn about this one.

It's not really the kind of hardening and not the exact test which gives me hick-ups, but rather the big picture: you're trying to package rdflib without SPARQLWrapper and then fix tests to make it work despite SPARQLWrapper being a requirement of rdflib (see setup.py).

I fear that going down this route will give us all kinds of problems in the future with different package management systems deciding "eh, this is not really a core part".

So my answer would be: the test is correct, it's reasonable to assume SPARQLWrapper is there. If the test fails it's a good thing as it tells you that something is wrong. If there are old tests which aren't as strict I'd consider them legacy code and would offer to make them more strict. So I'd suggest to package SPARQLWrapper for Fedora as well instead of fixing this test.

Or we decide that SPARQLWrapper is not a core requirement but optional for rdflib (one consistent decisions across all package managers).

No matter what decision we reach:
could you fix the commit message (it's not SPARQLStore but SPARQLWrapper which is not installed) (git commit --amend, then git push -f)

@gromgull
Copy link
Member

I sort of agree with Jörn here. We did at some point discuss whether things like SPARQLStore should be done with setuptools "extras" (essentially optional dependencies that enable some feature when installed) - but decided against at, as they are not very widespread and most people do no know they exist.

Is there a problem with packaging SPARQLWrapper for Fedora?

@dbs
Copy link
Contributor Author

dbs commented Feb 28, 2014

Jörn - right, I'll force push a fix for the commit message to match what I edited here so that things are at least consistent.

If SPARQLStore was an optional extra, then upgrading the existing rdflib packages would be a snap; we could upgrade rdflib to 4.1.0 and still provide the core RDF functionality that people know and love (such as rdfpipe!) along with the new enhancements for microdata / RDFa 1.1, work on providing both python2.x and python3 versions of the package to match the excellent work you've done providing 2/3 support, and then when SPARQLWrapper is packaged a host of additional functionality comes along for free.

Instead, right now SPARQLWrapper sits on our critical path: it has to pass all of the peer review and legal checks to be included in the distribution, as the bar for entry is much higher than the bar for a mere upgrade of an existing package. And while at a glance the licensing etc seems to be okay to me, I still need to create the spec file and the package, and then wait for peer review for the new package to be reviewed. And in Fedora, at least, new packages often sit waiting for peer review for a long time because there is a dearth of volunteers with available time :/

Given that of the 2,000+ tests, most pass, a handful are skipped, and only this one fails when SPARQLWrapper is not installed, it seemed that it really should be an optional component. For what it's worth (not much), I was quite surprised to find that RDFLib had added SPARQL support--that seemed out of scope for the package. That said, I agree with Jörn that making the tests fail fast and loud when SPARQLWrapper is not installed would be a stronger message about the importance of SPARQLWrapper than just skipping the majority of them.

I could patch our version of RDFLib to work around the problem in the short term, but in general we very much prefer to not tinker with upstream if at all possible. The upshot, though, is that Fedora might not get an updated version of RDFLib for a while, as my time is limited as well. (Poking around, it looks like Debian Sid and Ubuntu 13.10 still package RDFLib 2.4.2; at least Fedora has been at 3.2.3 for a while now!)

Trying to package RDFLib 4.1.0 on Fedora, I ran into a blocker with this test.
SPARQLWrapper is not yet packaged for Fedora, and unlike other tests which are
skipped if SPARQLWrapper is not installed, this one throws a fatal error. So
let's skip it like the others.

Signed-off-by: Dan Scott <dan@coffeecode.net>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling dcbfb3d on dbs:skip_sparqlstore into fdbb9e9 on RDFLib:master.

@gromgull
Copy link
Member

Right - I DO see Dan's point so I think we should make an exception this time, since we've already released 4.1 with most SPARQLStore test disabled.

And I would like to get the largest number of people on 4.X ASAP, to reduce the support load.

I propose the following:

  • We merge this PR
  • See if any other issues since 4.1 can be fixed quickly
  • Make sure SPARQLStore says "pip install sparqlwrapper" when it dies and the dependency is not there.
  • Release a 4.1.1 within the next few days
  • THEN - fix up all the SPARQLStore tests, and simple fail if SPARQLWrapper is missing, so that the next release 4.2 or whatever will really need SPARQLWrapper.

Dan, would this be ok? This would give you some time to get SPARQLWrapper through the distribution process.

@joernhees
Copy link
Member

👍

gromgull added a commit that referenced this pull request Feb 28, 2014
Skip SPARQLWrapper test if SPARQLWrapper is not installed
@gromgull gromgull merged commit f511ede into RDFLib:master Feb 28, 2014
@dbs
Copy link
Contributor Author

dbs commented Mar 3, 2014

Thanks so much!

dbs added a commit to dbs/rdflib that referenced this pull request Mar 3, 2014
Per discussion in RDFLib#359

Signed-off-by: Dan Scott <dan@coffeecode.net>
dbs added a commit to dbs/rdflib that referenced this pull request Mar 3, 2014
Per discussion in RDFLib#359

Signed-off-by: Dan Scott <dan@coffeecode.net>
dbs added a commit to dbs/rdflib that referenced this pull request Mar 3, 2014
Per discussion in RDFLib#359
make SPARQLWrapper an extra_requires for the 4.1 release
series, enabling distribution packagers to upgrade to 4.1
for core RDFLib functionality while creating the required
SPARQLWrapper package for 4.2.

Signed-off-by: Dan Scott <dan@coffeecode.net>
@dbs dbs mentioned this pull request May 9, 2014
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

4 participants