Skip to content

Conversation

facelessuser
Copy link
Collaborator

Main reason is this change allows this to work with Pyinstaller (I currently have to maintain a fork just to have this small change). But also, the API is less likely to change and probably the more appropriate way to access Pygments.

@waylan
Copy link
Member

waylan commented Oct 5, 2014

Guess I don't understand the problem. Why is your change better? What difference does it make if I call a function that returns an instance of a class or if I create an instance of the same class myself. If anything, creating the instance myself should be less code which should be more performant (granted the difference would be negligible here). I don't know much about pyinstaller, but the big feature they advertise (with a quick look at their site) is that it just works with standard python - no tricks. Seems like a bug in their package to me.

Btw, I wrote the Pygments code many years ago. It is entirely possible that they changed their API and their currently recommended approach was not possible back then. I don't know. I haven't looked recently.

@facelessuser
Copy link
Collaborator Author

Has to do with hidden imports. Pyinstaller can resolve normal imports just fine, but dynamic importing cannot be easily predicted. For this reason they have a system for educating Pyinstaller on hidden imports, but the way Pygments is doing stuff with their mapping is a bit odd.

When packaged with Pyinstaller, it cannot find TextLexer. It can't see it in pygments.lexers.TextLexer because it is actually in pygments.lexers.special.TextLexer. Due to their mapping and such in a normal Python environment it can be seen, but Pyinstaller can't resolve it proper because pygments.lexers.TextLexer doesn't exist. I guess there may be a way with their hook system to possibly get it figured out, but I have yet to be able to do it.

With that said, using the API lets Pygments resolve its own mapping and the module imports correctly. It seems safer to use the API so when/if they change there infrastructure, you should always be able to get the correct lexer or formatter. Pygments should be allowed to handle their module abstraction which is why I figured they added an API as well as for connivence. The user should not need to know where moduleX is, they should use the API.

Anyways, I just figured switching to API more be more reliable in the long run and give the added benefit of working with Pyinstaller.

@facelessuser
Copy link
Collaborator Author

Anyways, if worse comes to worse, I found a way to get pyinstaller to work without this even though it is extremely ugly and non-ideal.

My goal was to remove the requirement for me to maintain a forked version of the markdown library so that I could remove it from my repo. The solution, sadly, is just to create a hook to replace codehilite with what I posted above. Maybe I'm just not smart enough to resolve this properly with a pyinstaller hook. But this would at least only require me to maintain a copy of codehilite and could use the installed Python-Markdown on the system, but it will be strongly tied to specific versions, but that is fine as this is a personal project:

def hook(mod):
    # Replace mod by fake 'codehilite' module.
    hook_dir = os.path.abspath(os.path.dirname(__file__))
    fake_file = os.path.join(hook_dir, 'fake', 'fake-codehilite.py')
    new_code_object = PyInstaller.utils.misc.get_code_object(fake_file)
    mod = PyInstaller.depend.modules.PyModule('markdown.extensions.codehilite', fake_file, new_code_object)
    return mod

If TextLexer was referenced either directly as pygments.lexers.special.TextLexer or it was acquired via the API as I have done above (which I think has the potential to be safer long term), the problem can be resolved without such a sloppy, brute force hook. Pyinstaller resolves pygments.lexers.special.TextLexer just fine on its own once it it is added as a hidden_import (as this is the actual location of the module), and it can retrieve the module via its mapping fine through the API (as the API knows how to deal directly with the mapping object to get to pygments.lexers.special.TextLexer), but Pyinstaller just doesn't have the intelligence to resolve the mapping object that pygments.lexers directly returns for TextLexer in this indirection when packing the environment.

I would love to see this get pulled, or even just TextLexer get referenced from pygments.lexers.special as that seems sufficient, but I do have a backup plan now.

@waylan
Copy link
Member

waylan commented Oct 7, 2014

When packaged with Pyinstaller, it cannot find TextLexer. It can't see it in pygments.lexers.TextLexer because it is actually in pygments.lexers.special.TextLexer. Due to their mapping and such in a normal Python environment it can be seen, but Pyinstaller can't resolve it proper because pygments.lexers.TextLexer doesn't exist. I guess there may be a way with their hook system to possibly get it figured out, but I have yet to be able to do it.

Ah, so Pygments is using some "magic" which is tripping up Pyinstaller. Now that is useful info to me.

Also, after reviewing the Pygments docs, I couldn't help but notice that pygments.lexers.get_lexer_by_name and pygments.formatters.get_formatter_by_name are listed under the "High-level API," whereas the classes themselves are not. Personally, I would prefer creating the class instances myself (less magic is a good think), but that is clearly not the way they intend Pygments to be used.

Given the above two points, I think it probably makes sense to make this change.

waylan added a commit that referenced this pull request Oct 7, 2014
@waylan waylan merged commit 57633f1 into Python-Markdown:master Oct 7, 2014
@facelessuser
Copy link
Collaborator Author

Thanks a lot.

I get what you mean with less magic better, it is certainly faster and more direct. I actually deal a lot with embedded systems where magic/abstraction is necessary to support different kinds of hardware without breaking higher layers, so I think I fall somewhere in the middle.

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.

2 participants