immerse-project / nemo-simsar

Tools and guides how to share NEMO user configurations & experiments with others using a git repository.
6 stars 0 forks source link

REVIEW: use of python svn 1.0.1 #23

Open mscheinert opened 4 years ago

mscheinert commented 4 years ago

Describe the bug It seems the svn module (svn.local) is only used to import the exceptions svn.exception.SvnException after a subprocess call, e.g.:

https://github.com/immerse-project/nemo-simsar/blob/9377f51f8fff20dad09ff77d837cb65579bc5d9b/src/mkReadme.py#L377-L388

If we find a workaround for this, we could drop the svn module (GPL-2) completely...

Additional context The svn python package is distributed under GPL-2 which has implications for products linking to this library! (It's not the LGPL!)

mscheinert commented 4 years ago

Is this working at all? @lmicallef , please could you verify that?

The only case, where it would make sense is in check_gitorsvn(): https://github.com/immerse-project/nemo-simsar/blob/9377f51f8fff20dad09ff77d837cb65579bc5d9b/src/mkReadme.py#L377-L388

where we have to check, whether the svn command raises an error or not. If repo.returncode > 0 then there is a problem with the working copy and it's therefore not a valid svn repo. But it schould be sufficient to replace the SvnException with a subprocess.CalledProcessError and make sure, the error-message comes from svn. Something like this:

try:
    subprocess.check_output("svn info", stderr=subprocess.STDOUT, shell=True)
except subprocess.CalledProcessError as e:
    if e.output.decode("utf-8").startswith('svn: '):
        print(BLUE + "This is not an svn repo \n" + NORMAL)
    else:
        print("Error - ", e)
except OSError as e: 
    print("Error - ", e)
else:
    print(BLUE + "This is an SVN Repo" + NORMAL)

The exceptions in get_currepo():

https://github.com/immerse-project/nemo-simsar/blob/9377f51f8fff20dad09ff77d837cb65579bc5d9b/src/mkReadme.py#L569-L589

and get_nemorevision():

https://github.com/immerse-project/nemo-simsar/blob/9377f51f8fff20dad09ff77d837cb65579bc5d9b/src/mkReadme.py#L636-L645

could be handled in a similar way using the subprocess.CalledProcessError exception but wihtout a svn specific test.

lmicallef commented 4 years ago

@mscheinert, I tested subprocess.CalledProcessError in check_gitorsvn() and it does work. I will work on get_currepo() and get_nemorevision() to include subprocess.calledProcessError instead of svn.exception.SvnException. If the latter works too, I agree we can remove the svn.local module.

lmicallef commented 4 years ago

get_currepo() and get_nemorevision() seem to working fine with subprocess.calledProcessError. We can eliminate svn.local module.

lmicallef commented 4 years ago

The latest script reflects this change.