Skip to content
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

Install data files from the Makefile #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xdbob
Copy link
Contributor

@xdbob xdbob commented Oct 19, 2023

This commit reverts 74e8aa8 and part of f2491ba.

Fixes #31

This commit reverts 74e8aa8 and part of
f2491ba.

Signed-off-by: Antoine Damhet <antoine.damhet@lse.epita.fr>
@roddhjav
Copy link
Owner

The reason behind the current install is to provide a fully featured local install using pip, which is quite standard for python program.

As the extension entry point is in audit.bash, it needs to be part of the pip packaging too. This explain why I went for the current structure. As your PR would break this, could you advise on the best modern way to do it?

@xdbob
Copy link
Contributor Author

xdbob commented Oct 20, 2023

I've scratched my head multiple times on this kind of problem, I don't think there is a good way to do it (I may have missed it tho).

IMHO: the less-worse solution is something like this PR, eg a full separation of the python module from the system files. It has the added benefit to allow the user to swap the python module without the sytem files.

The second less-worse solution is to bundle the data files into the python module and provide install/uninstall functions where the user would be expected to do something like:

$ pip install pass-audit
$ python -m pass_audit install

@anarcat
Copy link
Contributor

anarcat commented Feb 18, 2024

i do not share @xdbob's interpretation in #31 where it is basically argued the only way to ship data files is with a Makefile, quoting this part of the setuptools guide.

The actual article, however, basically says that, to ship "data files", you must ship them inside your package which, in the case of the Debian package for this, is in /usr/lib/python3/dist-packages/pass_audit, hardly something man could find... It seems like this is something that's being more and more left by the wayside in Python packaging: just doing a pip install and expecting that to have a global impact is less and less supported (and, arguably, for good reasons).

This is discussed at length in pypa/packaging-problems#72 as well. It looks like the latest entry there is a reference to the shared-data wheel option, showing this example:

https://github.com/jupyter/notebook/blob/4e6282c1e522dd6358ac0235365885188a2ca031/pyproject.toml#L83-L86

It's similar to what you're doing now, but with hatchling, under the [tool.hatch.build.targets.wheel.shared-data] heading. I suspect there should be Something That Works in setuptools as well, but I can't figure it out right now.

I am actually surprised this doesn't work in wheels. I maintain the feed2exec package as an upstream, and it ships bash completions through setuptools, as such:

https://gitlab.com/anarcat/feed2exec/-/blob/7452109b1b5e28a44d4856d9db3c0139be0f6c37/setup.cfg#L61-63

@xdbob are you saying that if you pip install feed2exec globally right now, you don't get the bash completion? i find that surprising!

@xdbob
Copy link
Contributor Author

xdbob commented Feb 18, 2024

i do not share @xdbob's interpretation in #31 where it is basically argued the only way to ship data files is with a Makefile, quoting this part of the setuptools guide.

I didn't argued it's the only way but the less worse :)

The actual article, however, basically says that, to ship "data files", you must ship them inside your package which, in the case of the Debian package for this, is in /usr/lib/python3/dist-packages/pass_audit, hardly something man could find... It seems like this is something that's being more and more left by the wayside in Python packaging: just doing a pip install and expecting that to have a global impact is less and less supported (and, arguably, for good reasons).

That is precisely my point, pass-audit must install files outside of the python site-package (dist-package in the debian world).

This is discussed at length in pypa/packaging-problems#72 as well. It looks like the latest entry there is a reference to the shared-data wheel option, showing this example:

https://github.com/jupyter/notebook/blob/4e6282c1e522dd6358ac0235365885188a2ca031/pyproject.toml#L83-L86

It's similar to what you're doing now, but with hatchling, under the [tool.hatch.build.targets.wheel.shared-data] heading. I suspect there should be Something That Works in setuptools as well, but I can't figure it out right now.

I think it's hatch specific, it may be worth to look deeper into it tho.

I am actually surprised this doesn't work in wheels. I maintain the feed2exec package as an upstream, and it ships bash completions through setuptools, as such:

https://gitlab.com/anarcat/feed2exec/-/blob/7452109b1b5e28a44d4856d9db3c0139be0f6c37/setup.cfg#L61-63

@xdbob are you saying that if you pip install feed2exec globally right now, you don't get the bash completion? i find that surprising!

Indeed it works :) You appear to use [options.data_files] in your setup.cfg but setuptools actively discourages it's use and says it's deprecated.

If needed I am willing to try if [options.data_files] works in the fedora build system. In any case thanks @anarcat for looking into this headache

@roddhjav
Copy link
Owner

Thanks @anarcat and @xdbob for the help.

It is actually a bigger issue than shipping completion file. Part of the "data files" is the extension entry point audit.bash. It needs to be shipped at the good location, otherwise the extension will simply not work:

  • /usr/lib/password-store/extensions/audit.bash for system install
  • ~/.password-store/extensions/audit.bash for local install

Doing it in the Makefile would break all install using pypi.

Having an init script that do something like python -m pass_audit install at the first rule may be indeed the best (less ugly) option.

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.

Don't use python to ship data files
3 participants