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

README.md file in package is installed in venv outside the package subdirectory #295

Closed
huonw opened this issue May 22, 2023 · 3 comments · Fixed by #296
Closed

README.md file in package is installed in venv outside the package subdirectory #295

huonw opened this issue May 22, 2023 · 3 comments · Fixed by #296
Assignees
Labels
question Further information is requested

Comments

@huonw
Copy link
Contributor

huonw commented May 22, 2023

The README.md file is explicitly packaged into the wheel, but is outside the cabin directory. This means it ends up installed directly within site-packages/ when installed, rather than directly associated with casbin. An unscoped loose file like this can cause confusion and can interfere with other packages that also have a similar mistake (e.g. pex-tool/pex#1594), including other pycasbin packages like https://github.com/pycasbin/sqlalchemy-adapter.

For example:

cd $(mktemp -d)

python -m venv venv
. venv/bin/activate

# before:
ls venv
#> bin		include		lib		pyvenv.cfg

pip install casbin==1.18.2

# after:
ls venv    
#> README.md	bin		include		lib		pyvenv.cfg

head -4 venv/README.md 
#> PyCasbin
#> ====
#> 
#> [![GitHub Action](https://github.com/casbin/pycasbin/workflows/build/badge.svg?branch=master)](https://github.com/casbin/pycasbin/actions)

Notice that the top-level of the venv directory includes casbin's README. This means that installing casbin will add extraneous files to the venv, without an obvious source.

As mentioned, these sort of extraneous files interfere when using tools designed to give reproducible installation, because there's no way to automatically resolve the conflicts, if two packages install the same file. For this particular file, I imagine it doesn't matter if casbin_sqlalchemy_adapter's README overwrites casbin's one, but it would be better to keep each package self-contained.

Proposed fix: don't include the README as a data file.

I suspect the readme doesn't need to be included at all, because its contents is already contained in the wheel, via the long description, in the package metadata file (casbin-.../METADATA): unzip -p ~/Downloads/casbin-1.18.2-py3-none-any.whl casbin-1.18.2.dist-info/METADATA | head -30. If required, this can even be accessed from code via import importlib.metadata; importlib.metadata.metadata("casbin").get_payload().

This seems to apply to other pycasbin modules too: https://github.com/search?q=org%3Apycasbin%20data_files&type=code


Thanks for Casbin!

@casbin-bot
Copy link
Member

@techoner @Nekotoxin

@casbin-bot casbin-bot added the question Further information is requested label May 22, 2023
@hsluoyz
Copy link
Member

hsluoyz commented May 22, 2023

@huonw thanks for this!

  1. Can README be put into casbin folder?
  2. How to resolve this issue, can you make a PR?

@huonw
Copy link
Contributor Author

huonw commented May 22, 2023

  1. Can README be put into casbin folder?

As an alternative, are you aware that the README contents is already included in the wheel's METADATA file (because of the long_description? For example, running head -30 venv/lib/python3.9/site-packages/casbin-1.18.2.dist-info/METADATA shows:

Metadata-Version: 2.1
Name: casbin
Version: 1.18.2
Summary: An authorization library that supports access control models like ACL, RBAC, ABAC in Python
Home-page: https://github.com/casbin/pycasbin
Author: TechLee
Author-email: [email protected]
License: Apache 2.0
Keywords: casbin,acl,rbac,abac,auth,authz,authorization,access control,permission
Classifier: Programming Language :: Python :: 3.6
Classifier: Programming Language :: Python :: 3.7
Classifier: Programming Language :: Python :: 3.8
Classifier: Programming Language :: Python :: 3.9
Classifier: Programming Language :: Python :: 3.10
Classifier: License :: OSI Approved :: Apache Software License
Classifier: Operating System :: OS Independent
Requires-Python: >=3.3
Description-Content-Type: text/markdown
License-File: LICENSE
Requires-Dist: simpleeval (>=0.9.11)

PyCasbin
====

[![GitHub Action](https://github.com/casbin/pycasbin/workflows/build/badge.svg?branch=master)](https://github.com/casbin/pycasbin/actions)
[![Coverage Status](https://coveralls.io/repos/github/casbin/pycasbin/badge.svg)](https://coveralls.io/github/casbin/pycasbin)
[![Version](https://img.shields.io/pypi/v/casbin.svg)](https://pypi.org/project/casbin/)
[![PyPI - Wheel](https://img.shields.io/pypi/wheel/casbin.svg)](https://pypi.org/project/casbin/)
[![Pyversions](https://img.shields.io/pypi/pyversions/casbin.svg)](https://pypi.org/project/casbin/)
[![Download](https://img.shields.io/pypi/dm/casbin.svg)](https://pypi.org/project/casbin/)

This can even be accessed programmatically using import lib.metadata, e.g.:

from importlib.metadata import metadata
print(metadata("casbin").get_payload())
#> PyCasbin
#> ====
#> 
#> [![GitHub Action](https://github.com/casbin/pycasbin/workflows/build/badge.svg?branch=master)](https://github.com/casbin/pycasbin/actions)
#> ...

Why do you want to include the README file itself in the wheel? I imagine that most people would never look at the file structure and notice the README exists.

  1. How to resolve this issue, can you make a PR?

I've filed #296 and pycasbin/sqlalchemy-adapter#63 (the packages we use), but, as mentioned in the issue, other repositories are affected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants