Page MenuHomePhabricator

templates: add __init__.py files to templates/ dirs
ClosedPublic

Authored by martinvonz on Jul 31 2020, 7:59 PM.

Details

Summary

This is necessary for them to be loaded with importlib.resources,
which we want to do for PyOxidizer and similar. importlib.resources
cannot read resources from submodules
(resources.open_binary('mercurial.templates', 'coal/map') is not
valid), so we need one __init__.py per directory.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Jul 31 2020, 7:59 PM
indygreg accepted this revision.Aug 2 2020, 12:24 PM
indygreg added a subscriber: indygreg.

OMG. When I reviewed this, I thought your assertion that resources.open_binary('mercurial.templates', 'coal/map') is not valid was wrong because the low-level resource reader on the meta path importer does allow relative paths! However, it appears that the open_binary() helper functions limit this! This is yet another example of wonky behavior in these resources APIs. FWIW I've brought many of these to the attention of the core Python people at https://bugs.python.org/issue36128 and https://gitlab.com/python-devs/importlib_resources/-/issues/58 (and a few other places that I can't find the links to).

Anyway, we could potentially avoid having __init__.py files if we bypassed the importlib.resources helper functions and accessed the APIs on mercurial.__loader__ instead. I'll hold off queuing this part of the series to give you time to contemplate that (possibly simpler) alternative. I will say that a benefit to importlib.resources is they are higher-level and should abstract any changes to the low-level importer/loader interface. (The Python people were considering yet another API in Python 3.9, although I'm not sure if it was finalized.)

This revision is now accepted and ready to land.Aug 2 2020, 12:24 PM

OMG. When I reviewed this, I thought your assertion that resources.open_binary('mercurial.templates', 'coal/map') is not valid was wrong because the low-level resource reader on the meta path importer does allow relative paths! However, it appears that the open_binary() helper functions limit this! This is yet another example of wonky behavior in these resources APIs. FWIW I've brought many of these to the attention of the core Python people at https://bugs.python.org/issue36128 and https://gitlab.com/python-devs/importlib_resources/-/issues/58 (and a few other places that I can't find the links to).

Huh, I remember reading that bug report a while ago, but I had forgotten about it when I was working on this series.

Anyway, we could potentially avoid having __init__.py files if we bypassed the importlib.resources helper functions and accessed the APIs on mercurial.__loader__ instead. I'll hold off queuing this part of the series to give you time to contemplate that (possibly simpler) alternative. I will say that a benefit to importlib.resources is they are higher-level and should abstract any changes to the low-level importer/loader interface. (The Python people were considering yet another API in Python 3.9, although I'm not sure if it was finalized.)

I consider you the expert on this, so I'll let you decide. So you're saying we could rewrite our mercurial.utils.resourceutil module so it uses mercurial.__loader__ instead of importlib.resources and then we can drop this patch completely, and also remove the helptext/__init__.py and defaultrc/__init__.py files that we've added earlier? That sounds nice. I assume that will work on PyOxidizer, but is it possible that we might break similar tools? Would it be according to spec for another frozen-binary-tool to create a separate __loader__ for non-package submodules? Even *if* that's allowed according to the specs, I trust you if you tell me that it's unlikely enough that we shouldn't worry about it.

Having thought about this a bit more, I think using the importlib.resources high-level APIs are better. i.e. this series as-is is fine.

Since Python is talking about yet another low-level loader API, presumably the high-level APIs will be forwards compatible with whatever new low-level APIs are introduced in the future. This future proofs Mercurial.

The downside, unfortunately, is we need to introduce extra packages/__init__.py files. But this feels like a small price to pay.

This revision was automatically updated to reflect the committed changes.