Skip to content

fix: Ensure that repository access is resilient against namespace packages #106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mitchnegus
Copy link
Member

@mitchnegus mitchnegus commented Apr 4, 2025

We install MCs by registering the MC repo's __path__ attribute as an entry point. That __path__ attribute is a sequence, and FIREWHEEL currently grabs the first item in that sequence to be the repository path.

However, it appears that when an MC package repo does not have a top-level __init__.py (e.g., firewheel_repo_dns) and it is installed in editable mode, that package is treated as a namespace package. In this case, the path that we would like to register via the entry point is not necessarily the first item in the sequence (it looks like the first entry is the empty placeholder directory in the site-packages location, and the second entry is / may be the path to the actual modules).

I've put a bit of thought into how to resolve this problem, and my thinking is that there might not be a downside to including every item in the __path__ sequence. For one thing, this way we don't have to make any assumptions about where in our sequence the path we really want will be. It could be first, second, last, wherever. Path elements of __path__ that do not have MC modules are ignored as repositories by the mc list helper. And, in the case that someone actually does make an MC repo a real namespace package (e.g., a package where the components are not actually in the same repo, I think), we will still include all of the elements.

An alternative is that we just include an __init__.py at the top-level all of our MC package repos, but the missing one in firewheel_repo_dns suggests that this might be a route that persistently gives rise to tricky bugs if that empty, often-not-required, seemingly useless file is missing. We'd need to make sure that gets documented really well.

@github-actions github-actions bot added the fix Something isn't working label Apr 4, 2025
@mitchnegus mitchnegus marked this pull request as draft April 4, 2025 02:14
@mitchnegus mitchnegus force-pushed the namespace-packages branch 2 times, most recently from 5377f66 to f563165 Compare April 9, 2025 19:25
@gregjacobus
Copy link
Collaborator

It's unclear to me in what case the current implementation fails. I cloned firewheel_repo_dns, did a pip install -e ., and everything looks to be installed correctly. And there's only one item in the path:

>>> import firewheel_repo_dns
>>> firewheel_repo_dns.__path__
_NamespacePath(['/opt/dns/src/firewheel_repo_dns'])

That doesn't mean that this shouldn't still be merged, but I'd like to understand the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants