Inkscape_r2p: Fix imports for Python 2/3

[tbpassin]

Add absolute_import, change import syntax.

[tbpassin]

This change now passes a basic import test without failing:

py38 -m rst2pdf.extensions.inkscape_r2p  # or py27  

[akrabat]

Can you explain what problem this solves please?

[tbpassin]

Rob, this change allows the inkscape_r2p.py module to be imported and run in both Python 3 and Python 2.7, regardless of the current working directory from which it is invoked . I don't know exactly how this extension is used in practice, but presumably someone needed it at some time in the past.

I've been going through the code base one module at a time, to get them all to at least import with Python 3+. I started on this because I wanted to build a pdf document via Sphinx, and I found that I had to adjust a number of files to make that happen. So I decided to work through all of them.

I'm aware that not all of the modules are needed to make my particular document build successfully, but I'm going through them all anyway. I picked a few modules with simple changes so I could learn how to use the clone/change/pull request process, and possibly learn how to work up a test case that could be used with autotest on linux. But bear in mind that it's possible that some errors on Windows don't cause failures on Linux. These cases tend to involve paths to files. I don't know if there are any in rst2pdf, but I found one in Jinja2 that prevented my document from building on Windows.

I'm working on Windows, and the autotest suite doesn't run on Windows. So I have been making sure that at a minimum, all modules can import and run. See my next comment for how to repeat these tests.

[tbpassin]

Here is the test procedure that I have been using on Windows.

1. Clone the rst2pdf repo. Call its location on your computer REPO.

2. Open a console and change to some directory that is not REPO. Working from a directory outside REPO catches assumptions in at least one module (dumpstyle) about where directories are found.

3. Set the PYTHONPATH environmental variable to REPO. This causes Python to load modules from REPO first, so you can test any changed versions. On Windows:

    set PYTHONPATH=REPO

4. Try to run the module in both python 2.7 and python 3+. MODULE is the name of the module to be tested.

    py27 -m MODULE
    py38 -m MODULE

If no errors are displayed on the console, then MODULE does not need any changes. Typically, if a module fails it will be because the imports of other modules don't work right. Trying to run the master branch version of vectorpdf_r2p module, I got the following:

    C:\Users\tom>py38 -m rst2pdf.extensions.inkscape_r2p
    Traceback (most recent call last):
    File "C:\Users\tom\AppData\Local\Programs\Python\Python38\lib\runpy.py", line 192, in   _run_module_as_main
    return _run_code(code, main_globals, None,
    File "C:\Users\tom\AppData\Local\Programs\Python\Python38\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
    File "D:\Tom\git\rst2pdf\rst2pdf\extensions\inkscape_r2p.py", line 19, in <module>
    from vectorpdf_r2p import VectorPdf
    ModuleNotFoundError: No module named 'vectorpdf_r2p'

'vectorpdf_r2p' was not found because the import statement has to be adjusted.

5. Change your cloned repo to the branch with the fixes. I do this using GitHub Desktop on Windows, which makes this branch change very easy. Then repeat step 4. If the changes fixed the problem, then you will see no error traceback on the console.

[akrabat]

I've been going through the code base one module at a time, to get them all to at least import with Python 3+. I started on this because I wanted to build a pdf document via Sphinx, and I found that I had to adjust a number of files to make that happen. So I decided to work through all of them.

This is interesting to me as Sphinx works on Linux and Mac with the current code. Are you seeing Windows specific errors?

[tbpassin]

Yes, but let me clarify. Originally, I got errors using whatever version was installed by pip. I started to work on them, but at some point realized that I should be working with the Master branch, so I git-cloned it. There were still many files that wouldn't work with Python 3.8, but they may not have been used by my Sphinx build.

There was still one module that wouldn't work, but it was in Jina2, not rst2pdf. And it was a path problem. In Jinja2 , templates are identified by something that looks like a path to the template file, but is actually a canonical Linux form using "/" separators. This is supposed to not be a path, but in fact, at the point when the template was being invoked, the canonical form was getting used and not the Windows form. So the template file couldn't be found, and the build would error out.

I fixed the Jinja2 file so that it corrected the template path to use the platform path separator, and then my entire build succeeded. I'm not sure whether rst2pdf sends the Linux form to Jinja2 or not. If so, that's something that could be corrected in rst2pdf. Otherwise, it's on the Jinja folks to fix it. I posted a note about this problem to their site, but who knows if they will do anything.

The failure occurred when Jinja2 was looking for the rst2pdf template cover.tmpl. I suppose that after that it would have looked for the sphinx template, but it didn't get that far.

You would not have seen this problem on Linux, because the canonical form and the actual paths are the same.

So as I see it, the current Master branch will build my Sphinx document under Python 3.8 provided that I use the fixed file in Jinja2. How to make sure that other people who use Windows can get this working, I'm not sure. I would think that it would be worth looking at how the the rst2pdf request for this template gets to Jinja2, in case it can be fixed. If you could point me to the module where the templates are passed to Jinja2, I could take a look at it.

But really, the Jinja2 folks should fix their code and then the problem won't arise again.

[lornajane]

I can replicate the test failures that caused the build on this PR to fail so I'll try to take a closer look at those to understand if they should block progress of this PR or not.

[lornajane]

The four failing tests are:

• test_fancytitles
• test_foreground
• test_inkscape
• test_issue_239_2

Checking their logs, they all have:

* [INFO] createpdf.py:1578 Importing extension module 'inkscape'
* 
* Error: Could not find module inkscape in sys.path [ ...]

in their .log files, and didn't produce a PDF. It's similar to the error I see in doing python -m rst2pdf.extensions.inkscape_r2p with python 3 on the master branch - which this PR fixes. But we need to work out how to fix the path for the other extensions too, and I'm not sure how. It's not expressly included in the same way.

[tbpassin]

The test_fancytitles test specifies two extensions, fancytitles and the inkscape extension. Running it with only -e fancytitles returned no error:

d:\>py38 -m rst2pdf.createpdf -e fancytitles "D:\Tom\git\rst2pdf\rst2pdf\tests\input\test_extensions.txt"

So at least that part is OK.

Running with only -e inkscape_r2p failed. Here were the last lines of the traceback:

 File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
 File "D:\Tom\git\rst2pdf\rst2pdf\extensions\inkscape_r2p.py", line 20, in <module>
 from .vectorpdf_r2p import VectorPdf
 ImportError: attempted relative import with no known parent package

This has something to do with using the importlib.import_module vs just importing the module. My previous fix to inkscape_r2p added a leading dot to the import that fails:

from .vectorpdf_r2p import VectorPdf

We need that dot to import directly from the command line. But when I remove it, then this fancytitles test succeeds.

The action happens around line 1579 in createpdf.py. The routine tries to figure out several possible variations of the extension module name. There is a way to fix the import. The import_module call takes an optional second argument to designate the package for imports when the system doesn't know what the package should be, and the import is relative. The package will always be "rst2pdf". So the following changes to these imports succeeded for me:

   # The only functional change here is adding the package name 'rst2pdf'.
   # The other changes were made to clarify the loop, and provide for possibly
   # adding other name variations in the future.
    _names = [firstname, modname] 
    try:
        for _name in _names:
            try:
                module = import_module(_name, 'rst2pdf')
            except (ImportError, ModuleNotFoundError):
                continue
    except ImportError as e:
         # Continue with existing code here

Actually, although all the imports succeeded, the whole test still failed because it wants to find a template that was not included with my "git clone" tree. The template is titletemplate.svg.

So it seems that adding the 'rst2pdf' package name to the import_module call will do the job. I'd say the package name should be pretty stable, since it's hard to see that anyone would install rst2pdf under a different name. But if someone thinks that could be a problem, I suppose it could be parameterized in some way.

[tbpassin]

The above fix to createpdf solved the import problems for all four test cases (only tested with Python 3.8, though). test_issue_239_2 still failed, but not for the import problems. The problem occurs in image.py, which can't figure out how to process the svg test image:

d:\>py38 -m rst2pdf.createpdf -e inkscape D:\Tom\git\rst2pdf\rst2pdf\tests\input\test_issue_239_2.txt
D:\Tom\git\rst2pdf\rst2pdf\rson.py:140: FutureWarning: Possible nested set at position 2 
splitter = re.compile(pattern).split
[ERROR] image.py:151 Couldn't load image [D:\Tom\git\rst2pdf\rst2pdf\tests\input\warning.svg]

Around line 151, the code is trying to figure out who can process this kind of image. The last attempt is PIL, and that didn't work out either:

        # PIL can't, so we can't
        self.support_warning()
        log.error("Couldn't load image [%s]"%filename)

I do have both PIL and pillow installed, so that's not the problem.

But anyway, it looks like the import_module change to createpdf takes care of the import problems for Python 3+.

[tbpassin]

We have two types of uses to get working with the inkscape imports:

1. Used as an extension with createpdf:

   py38 -m rst2pdf.createpdf -e inkscape some-test-file-here  # or py27
   

2. Called directly:

   py38 -m rst2pdf.extensions.inkscape_r2p some-arguments-here  # or py27
   

To get both of these to work with both Python 2.7 and Python 3.8, I had to make the above change to createpdf (adding the 'rst2pdf' parameter to the import_module() call), and make the import in inkscape_r2p conditional:

try:
    from  vectorpdf_r2p import VectorPdf # works with Python 2.7
except:
    from  .vectorpdf_r2p import VectorPdf # works with Python 3.8

I realize (which I didn't at the beginning) that the second use case isn't actually needed for the inkscape module since it has no code that will run when called that way, but I can't help thinking that at least it should be possible, in case someone wants to add some self-test code or something. Anyway, there could be other modules that are usable directly, and this exercise has shown how to make them work under both version of Python.

[tbpassin]

Finally - with the above changes, all four of the tests passed (on Windows 10) both under both Python versions (2.7 and 3.8). By "passing" I mean that they completed with no errors. I didn't actually check the outputs. But with the imports working, it's hard to see why the outputs would have changed from the past.

[lornajane]

That sounds much more positive! Thanks for taking the time to look at this further. Do you want to push those changes that you needed to make to this branch so I can review them?

[tbpassin]

Will do ... I'll have to clean my branch up a bit first.

[tbpassin]

OK, I've pushed the changes to my GitHub repo, at https://github.com/tbpassin/rst2pdf/tree/inkscape_r2p/rst2pdf. They are on the inkscape_r2p branch. I didn't make another pull request because I wasn't sure how that would play with my original one.

There may have been some output pdfs pushed to the tests directory, I'm not sure. You can probably ignore them, since any testing you do will generate new ones anyway.

[lornajane]

I've run the test suite locally and it works perfectly on python3.7.6 (except for a weirdness with the fancy_titles test which is also present on the master branch) and on 2.7.15 one test fails to generate the PDF (test_issue_785), which I'm not too worried about.

@akrabat I think this change is safe enough, is there anything more we should do before we merge on this one?

[lornajane]

This was a lot of work for a small diff but it's an important change. Thankyou!