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 generic feature_download executable name #2227

Closed
sebastic opened this issue Aug 5, 2023 · 6 comments · Fixed by #2263
Closed

Don't use generic feature_download executable name #2227

sebastic opened this issue Aug 5, 2023 · 6 comments · Fixed by #2263

Comments

@sebastic
Copy link

sebastic commented Aug 5, 2023

Description

Since 0.22.0 tools/cartopy_feature_download.py gets installed as /usr/bin/feature_download which is a far too generic name.

The cartopy_ prefix should be preserved as per this patch:

Description: Don't use generic feature_download executable name.
Author: Bas Couwenberg <sebastic@debian.org>

--- a/pyproject.toml
+++ b/pyproject.toml
@@ -56,7 +56,7 @@ plotting = ["pillow>=6.1.0", "scipy>=1.3
 test = ["pytest>=5.1.2", "pytest-mpl>=0.11", "pytest-xdist", "pytest-cov", "coveralls"]
 
 [project.scripts]
-feature_download = "tools.cartopy_feature_download.py:__main__"
+cartopy_feature_download = "tools.cartopy_feature_download.py:__main__"
 
 [project.urls]
 documentation='https://scitools.org.uk/cartopy/docs/latest/'

Operating system

Debian unstable

Cartopy version

0.22.0

@greglucas
Copy link
Contributor

I agree this is not super ideal. More generally, I wonder if we even want to install "cartopy_feature_download" as a script like this at all because I don't think it rises to the level of needing to be site-wide available as a script. Maybe we could move it into a function within the package somehow instead. Any other thoughts on this from anyone?

@dopplershift
Copy link
Contributor

An option would be to have it as code in a __main__ in an appropriate module, so you could run it like python -m cartopy.feature ?

@QuLogic
Copy link
Member

QuLogic commented Aug 11, 2023

I think it used to be installed as cartopy_feature_download.py, so this rename was accidental.

@sebastic
Copy link
Author

I think it used to be installed as cartopy_feature_download.py, so this rename was accidental.

It was, and installing it without the language extension is better.

@greglucas
Copy link
Contributor

greglucas commented Aug 13, 2023

I think the current installation is actually broken? So a simple name change won't fix the underlying issue. It looks like setuptools expects scripts to be a part of the package, but we have put the script in a separate directory without an __init__.py so the tools directory isn't a module.

cartopy_feature_download gshhs physical --dry-run

/usr/bin/cartopy_feature_download", line 5, in <module>
    from tools.cartopy_feature_download.py import __main__
ModuleNotFoundError: No module named 'tools'

Note: we should switch CI to test the installed script instead of the direct path that we currently do with the dry-run if we do go this route to make sure we codify the expectation of how the script works.

@QuLogic
Copy link
Member

QuLogic commented Aug 14, 2023

Oh, that's a good catch; probably should move to cartopy.feature somewhere as @dopplershift suggested.

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 a pull request may close this issue.

4 participants