Split version info into a separate file, load it using imp/importlib

[mitya57]

To make sure markdown module is importable during installation.

It fixes Travis tests and also fixes python3 -m pep517.check, so I hope it closes #823.

[waylan]

I wonder if we should revert back to including the old get_version function we used to have in setup.py. The one concern is that the old solution explicitly avoided importing Markdown. Now that we include the version right in markdown/__init__.py its not possible to avoid importing Markdown. And I am not suggesting that we move the version back to a separate __version__.py file.

It's just that this feels like an dirty hack while that feels much cleaner as the module is never added to the PYTHONPATH and does not pollute any namespace. And as we used it for years, it is well tested and already developed.

[facelessuser]

I kind of agree with the old way. I currently do something very similar in my packages and haven't felt the need to change so far.

[waylan]

And while we're at is perhaps we might want to add python3 -m pep517.check path/to/source as a test env to tox. That should give us quick and easy to identify feedback if/when a problem is related to the build environment.

[mitya57]

Thanks for the link to the old code. I don't mind reverting to it, but in such case it would be nice to replace deprecated imp (which was used in setup.py) with importlib.

Otherwise the approach is basically the same as here — passing [path] to find_module has a similar effect to changing sys.path.

And also +1 for running pep517.check as part of CI.

[mitya57]

Replying to myself:

Thanks for the link to the old code. I don't mind reverting to it, but in such case it would be nice to replace deprecated imp (which was used in setup.py) with importlib.

It looks like there is no way to pass a path to importlib that will work with both Python 2 and Python 3. So we have these options:

• Use deprecated imp on Python 3 too (which is not nice);
• Use different code paths for different Python versions;
• Add the path to sys.path (and maybe remove it back after importing) — which will be equivalent to what I am doing in this PR;
• Drop support for Python 2;
• Do something very different (like extracting the version from file using regular expressions).

[waylan]

It looks like there is no way to pass a path to importlib that will work with both Python 2 and Python 3.

importlib.import_module is available in Python 2.7 as a simple wrapper around the old API for this very reason. Presumably we could adopt the Python3 solution now without dropping Python 2.7 yet.

[waylan]

And I just realized importlib.import_module won't work for us as it doesn't accept a file path. I suppose we could create a function with multiple paths through if statements or caught exceptions. But that seems ridiculous when dropping Python 2.7 is so immanent.

Let's just keep the existing change for now. We can reexamine when we drop Python 2.7. Add the pep517.check in this PR and I'll merge it.

[mitya57]

Add the pep517.check in this PR and I'll merge it.

Done.

[facelessuser]

While I'm not concerned with how this is ultimately solved here (as long as it works 🙂), recreating something similar to the old way without imp isn't too bad. I've been able to do it here: facelessuser/pymdown-extensions#569. In my case, I just left the old PY2 implementation under a conditional as I simply plan on ripping it out when PY2 is finally put down.

Anyways, just options.

[waylan]

If someone wants to do the work, I'm open to @facelessuser's suggestion.

[waylan]

facelessuser/pymdown-extensions#569.

I'm not crazy about that using exec. I'm open to a solution which does not use exec.

[mitya57]

The StackOverflow answer I posted above has a solution without exec (it uses importlib’s exec_module instead).

I can copy it here however I don’t see the benefits compared to my approach. Are there any?

[waylan]

I don’t see the benefits compared to my approach. Are there any?

As I stated before...

It's just that this feels like an dirty hack while that feels much cleaner as the module is never added to the PYTHONPATH and does not pollute any namespace.

[facelessuser]

The StackOverflow answer I posted above has a solution without exec (it uses importlib’s exec_module instead).

Either way is fine. Some people get uncomfortable using exec. I think in the proper situations it's fine, but I have no strong feelings either way.

I can copy it here however I don’t see the benefits compared to my approach. Are there any?

And as far as benefits over this pull 🤷‍♂ . I'm not personally using toml files yet, so I'm still hanging around with with how I was doing things previously. As the possibility of just importing a version file was discussed, I simply just wanted to provide how it could be done.

Ultimately, I'm not concerned with what gets done in the end. My preference is just importing the version stuff as a separate file, but if adding the cwd and importing markdown is preferred, I've no problems if it's working.

[facelessuser]

I just tried out spec_from_file_location from file, and it works out very well too. I think it is a much cleaner option. Since I don't think we are supporting Python 3.4, I don't see a reason to not use it, and just use imp for Python 2.

[mitya57]

If someone wants to do the work, I'm open to @facelessuser's suggestion.

Done. Please review the latest version.

[facelessuser]

LGTM

[waylan]

I'm not crazy about moving the version stuff back out to a separate file (I suspect it would have worked fine with that left in __init__.py) but it's fine. And __meta__.py makes sense as a name.

[mitya57]

I'm not crazy about moving the version stuff back out to a separate file (I suspect it would have worked fine with that left in __init__.py) but it's fine. And __meta__.py makes sense as a name.

It worked on Python 3 but not on Python 2:

Traceback (most recent call last):
  File "setup.py", line 46, in <module>
    __version__, __version_info__ = get_version()
  File "setup.py", line 36, in get_version
    markdown = imp.load_source('markdown', module_path)
  File "markdown/__init__.py", line 25, in <module>
    from .core import Markdown, markdown, markdownFromFile
ValueError: Attempted relative import in non-package

That’s why I had to split it into a separate file.

[waylan]

With a comment in the release notes, this should be good to go.

[mitya57]

With a comment in the release notes, this should be good to go.

Added.

[greut]

@mitya57 good job! much appreciated.