scmtools.svn: add subvertpy support, deprecating PySVN in docs

Review Request #5104 — Created Dec. 12, 2013 and submitted

Information

Review Board
master

Reviewers

Add subvertpy support, in addition to the existing PySVN support. This allows SVN users to use virtualenv + pip to install Review Board and its dependencies. If subvertpy is not installed, it falls back to PySVN.

Made the SVN implementation modular, so other libraries may be implemented if desired.

Made sure there were no regressions in the unit testsuite (Python 2.7, Linux), and made sure that PySVN still works.

Light manual testing (Python 2.7, Linux).

Description From Last Updated

Given that we require easy_install for our own packages, I think I'd rather not tell people to use pip for …

daviddavid

Your testing done said that you checked PySVN. Is this comment obsolete?

daviddavid

Can you make this an instance method instead of declaring it inline?

daviddavid

Why is this here? This doesn't seem to actually do anything...

daviddavid

Can we rewrite this line as: raise ImportError(_( 'SVN integration requires either subvertpy or pysvn'))

daviddavid

Can you include docstrings for all of the abstract methods here? I'd also like docstrings for the implementations in the …

daviddavid

This should be called accept_ssl_certificate. Please also add a docstring that explains what on_failure is.

daviddavid

Does subvertpy not do any SSL certificate checking? If we don't need this, can we at least: - change the …

daviddavid
MA
  1. There's one test failure that I'm not sure how to resolve, and I think it's nose-specific. It occurs when you don't have subvertpy installed:

    ======================================================================
    ERROR: Failure: ImportError (No module named subvertpy)


    Traceback (most recent call last):
    File "/home/mark/env/reviewboard/local/lib/python2.7/site-packages/nose/loader.py", line 413, in loadTestsFromName
    addr.filename, addr.module)
    File "/home/mark/env/reviewboard/local/lib/python2.7/site-packages/nose/importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
    File "/home/mark/env/reviewboard/local/lib/python2.7/site-packages/nose/importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
    File "/home/mark/Downloads/Code/reviewboard/reviewboard/scmtools/svn/subvertpy.py", line 6, in <module>
    from subvertpy import ra, SubversionException
    ImportError: No module named subvertpy

    It seems like nose loads modules differently from Review Board?

    1. How are you running the tests?

    2. ~/Downloads/Code/reviewboard $ ./reviewboard/manage.py test

    3. Well, that should be correct.

      Probably what we need to do is try/catch on the imports so that the file itself can be imported regardless of whether the backend class is actually usable or not. The current pysvn implementation does this, as do other SCMTools like bazaar.

      We should probably have a discussion about whether or not we want to retain pysvn compatibility or just switch over to subvertpy.

    4. Probably what we need to do is try/catch on the imports so that the file itself can be imported regardless of whether the backend class is actually usable or not. The current pysvn implementation does this, as do other SCMTools like bazaar.

      OK, now those try-except blocks make more sense. I will make those changes and submit an updated diff shortly.

      We should probably have a discussion about whether or not we want to retain pysvn compatibility or just switch over to subvertpy.

      Apart from backwards compatibility, the reason why I kept PySVN support in Review Board is because subvertpy has not yet released a Python 3-compatible version yet, although there has been a fair amount of work done to that end.

    5. If Subvertpy was a pure Python module, I'd say let's nuke PySVN and just depend on it. Since it still has to be compiled, and likely isn't packaged on many distros, I think having both implementations (and documenting just subvertpy for now) is the best way to go for the time-being, since it's far less work for those upgrading, and less likely to break working installations.

    6. subvertpy is actually packaged on most major distros that I know of, except RHEL/CentOS and related. If you want me to change the documentation to only mention subvertpy (right now my diff has both), that's easy enough, compared to fixing this import test...

    7. We still need the import test fixed, either way.

      I think the docs should encourage Subvertpy, but mention PySVN as a fallback. It's very possible we'll hit issues with Subvertpy that we're not aware of yet.

      RHEL/CentOS are pretty important, but I imagine once we have this support, they can get Subvertpy packaged fairly easily if they choose. I'm more concerned about older distros (not everyone upgrades their servers very regularly). I haven't looked into it though.

    8. We still need the import test fixed, either way.

      Sorry, I didn't mean to imply that I wasn't going to do that, it's just a little bit more involved than I had hoped.

      I think the docs should encourage Subvertpy, but mention PySVN as a fallback. It's very possible we'll hit issues with Subvertpy that we're not aware of yet.

      OK, I'll work on changing the docs to reflect this.

      RHEL/CentOS are pretty important, but I imagine once we have this support, they can get Subvertpy packaged fairly easily if they choose. I'm more concerned about older distros (not everyone upgrades their servers very regularly). I haven't looked into it though.

      When I made my statement about RHEL/CentOS, I was going off of the current state of the Review Board docs that say that PySVN isn't packaged in RHEL/CentOS as well. I just did a quick search, and both PySVN and subvertpy are not in the core RHEL repositories, but they are in the EPEL repository. I can update the docs to reflect this as well.

  2. 
      
MA
MA
david
  1. I'm sorry for the delay in reviewing this... I've been neck deep in rbtools refactoring for the last couple weeks.

    For the most part, this looks good. Most of these comments deal with documentation (both the user docs and docstrings), and a little bit of code style.

  2. docs/manual/admin/installation/linux.txt (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Given that we require easy_install for our own packages, I think I'd rather not tell people to use pip for things. Having multiple options is also confusing to novices who might not understand why there are two commands to install python modules. Can we just list the easy_install command?

  3. reviewboard/scmtools/svn/__init__.py (Diff revision 2)
     
     
    Show all issues

    Your testing done said that you checked PySVN. Is this comment obsolete?

    1. The # NOQA is for flake8. I'll change it to:
      # flake8: NOQA

    2. We don't use flake8. I' prefer not to litter the code with comments for linters.

  4. reviewboard/scmtools/svn/__init__.py (Diff revision 2)
     
     
    Show all issues

    Can you make this an instance method instead of declaring it inline?

  5. reviewboard/scmtools/svn/__init__.py (Diff revision 2)
     
     
     
    Show all issues

    Why is this here? This doesn't seem to actually do anything...

    1. (Didn't mean to make a separate review for this comment...)

      It's the equivalent code from the original function. It creates the client and makes sure that the SSL cert is valid.

    2. I forgot to remove this comment before publishing. I had figured it out after reading through the code more.

  6. reviewboard/scmtools/svn/__init__.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    Can we rewrite this line as:

    raise ImportError(_(
        'SVN integration requires either subvertpy or pysvn'))
    
  7. reviewboard/scmtools/svn/base.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can you include docstrings for all of the abstract methods here?

    I'd also like docstrings for the implementations in the backend modules. These can all be copied from the SCMTool class.

  8. reviewboard/scmtools/svn/base.py (Diff revision 2)
     
     
    Show all issues

    This should be called accept_ssl_certificate. Please also add a docstring that explains what on_failure is.

  9. reviewboard/scmtools/svn/subvertpy.py (Diff revision 2)
     
     
    Show all issues

    Does subvertpy not do any SSL certificate checking?

    If we don't need this, can we at least:
    - change the docstring here to be a comment
    - add 'pass' to the body

  10. 
      
MA
  1. 
      
  2. reviewboard/scmtools/svn/__init__.py (Diff revision 2)
     
     
     

    It's the equivalent code from the original function. It creates the client and makes sure that the SSL cert is valid.

  3. 
      
MA
david
  1. Ship It!
  2. 
      
MA
Review request changed
Status:
Completed
Change Summary:
Pushed to master (130be2f). Thanks!