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

Don't use the global data directory #901

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

FFY00
Copy link

@FFY00 FFY00 commented Aug 28, 2024

Putting data in the global data directory is not recommended, a better place for data generally is to install it as a subpackage. This PR makes that change.

Additionally, it's unclear to me how the moonraker config in update manager isn't broken, since requirements and system_dependencies specify set the base path to the Python package (moonraker.utils.source_info.source_path) but the files aren't installed there.

BASE_CONFIG: Dict[str, Dict[str, str]] = {
"moonraker": {
"origin": "https://github.com/arksine/moonraker.git",
"requirements": "scripts/moonraker-requirements.txt",
"venv_args": "-p python3",
"system_dependencies": "scripts/system-dependencies.json",
"virtualenv": sys.exec_prefix,
"pip_environment_variables": "SKIP_CYTHON=Y",
"path": str(source_info.source_path()),
"managed_services": "moonraker"
},
"klipper": {
"moved_origin": "https://github.com/kevinoconnor/klipper.git",
"origin": "https://github.com/Klipper3d/klipper.git",
"requirements": "scripts/klippy-requirements.txt",
"venv_args": "-p python3",
"install_script": "scripts/install-octopi.sh",
"managed_services": "klipper"
}
}

Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00 FFY00 changed the title scripts: move scripts to the Python package Don't use the global data directory Aug 28, 2024
@Arksine
Copy link
Owner

Arksine commented Aug 28, 2024

Putting data in the global data directory is not recommended, a better place for data generally is to install it as a subpackage.

Hi. I must ask, not recommended by whom? While I agree with this recommendation when it comes to data used by the package itself, this does not apply to the items in Moonraker's scripts path. They are external helpers, primarily used to help install Moonraker on a system. In my view the shared data directory is a viable location for these items. Moving the scripts path in the repo is not an option, as it would break existing workflows that install Moonraker from source.

Additionally, it's unclear to me how the moonraker config in update manager isn't broken, since requirements and system_dependencies specify set the base path to the Python package (moonraker.utils.source_info.source_path) but the files aren't installed there.

The path option is not used when updating python packages.

@FFY00
Copy link
Author

FFY00 commented Sep 5, 2024

See https://setuptools.pypa.io/en/latest/userguide/datafiles.html#non-package-data-files.

The shared data directory is leftover from old packaging tooling, and has a set of issues. It's difficult to support and is likely to be deprecated in sysconfig at some point in the future, and will likely be missing from newer APIs (source: I am the maintainer 😅). Installation tools should still be able to install the files, preserving backwards compatibility, but we'd recommend against its usage.

Moving the scripts path in the repo is not an option, as it would break existing workflows that install Moonraker from source.

I included a symlink in the PR for this reason.

@Arksine
Copy link
Owner

Arksine commented Sep 5, 2024

Thanks for the information. I'll accept your recommendation, however I would prefer to copy the scripts into the distribution during the build rather than move the scripts folder and symlink to it. This way I can exclude items that do not need to be distributed in the package. In addition the install script will need an update to correctly locate the scripts folder when Moonraker is installed as a package. I will create a superseding PR to accomplish this.

FWIW, if the intent is to deprecate the use of the data directory, I would recommend coordinating with PyPA. According to the Python Packaging User Guide the data folder is the correct location for scripts and such. The recommendation from setuptools seems to apply to files accessed at runtime by the package, which makes sense but does not apply here. The update_manager only accesses items in the scripts path when Moonraker is installed from source.

@Arksine
Copy link
Owner

Arksine commented Sep 6, 2024

Commit 8df9890 copies selected items from the scripts folder into the package during the build. Since this change required an update to the installer I published a patch release of Moonraker, v0.9.3.

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