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

g.extension: fix getting addons path if input JSON file doesn't exist #2717

Merged
merged 6 commits into from
May 5, 2023

Conversation

tmszi
Copy link
Member

@tmszi tmszi commented Dec 23, 2022

To determine which multi-addons install only the manual HTML page. Fixes #2706.

Removes -j flag (which generated JSON file containing the download URLs of the official Addons)

@tmszi tmszi added bug Something isn't working backport_needed CI Continuous integration Python Related code is in Python labels Dec 23, 2022
@tmszi tmszi added this to the 8.3.0 milestone Dec 23, 2022
@tmszi tmszi requested a review from wenzeslaus December 23, 2022 08:20
@nilason
Copy link
Contributor

nilason commented Dec 23, 2022

@tmszi Perhaps you could test with removing the exclusion line for Mac tests:

scripts/g.extension/testsuite/test_addons_download.py

The test was almost always failing, so I excluded it.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I don't know why the information is needed, why it is okay when it is not there, or why it can't be obtained in another way.

However, as far as fixing the failure in #2706, this PR does the job.

I'm not sure about the exception type and the wording of the message.

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved
scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved
@tmszi
Copy link
Member Author

tmszi commented Dec 23, 2022

I'm sorry, I don't know why the information is needed, why it is okay when it is not there, or why it can't be obtained in another way.

I understand your complaint and it is relevant, the quality of my PR depends on how much time I can invest in it.

Explanation is in the function doc string:

def filter_multi_addon_addons(mlist):
"""Filter out list of multi-addon addons which contains
and installs only *.html manual page, without source/binary
excutable module and doesn't need to check metadata.
e.g. the i.sentinel multi-addon consists of several full i.sentinel.*
addons along with a i.sentinel.html overview file.
:param list mlist: list of multi-addons (groups of addons
with respective addon overview HTML pages)
:return list mlist: list of individual multi-addons without respective
addon overview HTML pages
"""
# Filters out add-ons that only contain the *.html man page,
# e.g. multi-addon i.sentinel (root directory) contains only
# the *.html manual page for installation, it does not need
# to check if metadata is available if there is no executable module.
for addon in get_multi_addon_addons_which_install_only_html_man_page():
if addon in mlist:
mlist.pop(mlist.index(addon))
return mlist

If addons_paths.json file doesn't exist (due exceeded GitHub API limitation which is 60 request per IP address per 1 hour), multi-addons installation work, but for multi-addons which contains addons which install HTML manual page only is printed warning message line 1371 (is not relevant in this use case, we don't needed to check metadata for these addons, and therefore is used filter_multi_addon_addons func). We can skip this filtering and continue addon installation.

# Filter multi-addon addons
if len(mlist) > 1:
mlist = filter_multi_addon_addons(
mlist.copy()
) # mlist.copy() keep the original list of add-ons
# update tree
for name in mlist:
try:
desc = gtask.parse_interface(name).description
# mname = gtask.parse_interface(name).name
keywords = gtask.parse_interface(name).keywords
except Exception as error:
grass.warning(
_("No metadata available for module '{name}': {error}").format(
name=name, error=error
)
)
continue

But on the other hand, the end user will not encounter this bug under normal circumstances (outside the CI env), because the addons_paths.json file is always downloaded after the first use of g.extension module, and if the allowed GitHub API limit is exceeded, the new file addons_paths.json is not downloaded but the previous one is used.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this. It looks good, but the code needs a comment on why it is okay to return the empty list. Or the information can be in the function doc string describing its behavior.

The addons = [] statement at line 1287 should be really only before the last for loop. The new return should just return []. This makes is more clear what the code is supposed to do.

@ninsbl
Copy link
Member

ninsbl commented Mar 5, 2023

Given all the rate limit issues and improvements in git, maybe we should consider abandoning the API?

Here are some git commands we may use as subprocesses, that do - and that also reasonably fast - what we currently use the API for:

cd /tmp
git clone --no-checkout --depth=1 --filter=tree:0 https://github.com/osgeo/grass-addons
cd grass-addons
# Get a tree of files and directories
git ls-tree --name-only -r HEAD

# List branches
git ls-remote --heads

# Checkout a specific addon
git sparse-checkout init
git sparse-checkout set src/vector/v.surf.tps
git checkout grass8

And completely free of dependencies proprietary or hosting specific solutions...

@ninsbl
Copy link
Member

ninsbl commented Mar 6, 2023

Here is some related python code:

import os
import subprocess

from datetime import datetime
from pathlib import Path


def get_addons_list(url="https://github.com/osgeo/grass-addons", base_dir="/tmp"):
    base_dir = Path(base_dir)
    if not base_dir.exists():
        base_dir.mkdir(exist_ok=True, parents=True)
    subprocess.run(
        ["git", "clone", "--no-checkout", "--depth=1", "--filter=tree:0", url],
        cwd=base_dir,
    )
    return subprocess.check_output(
        ["git", "ls-tree", "--name-only", "-r", "HEAD"], cwd=base_dir / "grass-addons"
    )


start = datetime.now()
get_addons_list()
end = datetime.now()
print(end - start)

@ninsbl
Copy link
Member

ninsbl commented Mar 6, 2023

git subprocesses and sparse checkouts are probably also the way to go to replace SVN (#2834)

@nilason
Copy link
Contributor

nilason commented Mar 6, 2023

git subprocesses and sparse checkouts are probably also the way to go to replace SVN (#2834)

That looks very promising! I wouldn't mind having that for 8.3 :)

@ninsbl
Copy link
Member

ninsbl commented Mar 21, 2023

Please test #2895
Using git sparse checkout should circumvent the problem of missing change history...

@neteler
Copy link
Member

neteler commented Apr 16, 2023

@tmszi shall I bump this PR to 8.4.0?

@tmszi
Copy link
Member Author

tmszi commented Apr 17, 2023

shall I bump this PR to 8.4.0?

Let's keep it at version 8.3.0, please. I'll try to implement @ninsbl suggestions as soon as possible and test it.

@tmszi tmszi force-pushed the fix-get-addons-paths-if-file-does-not-exist branch 2 times, most recently from af6d5f7 to 69b8f86 Compare April 19, 2023 18:17
@tmszi
Copy link
Member Author

tmszi commented Apr 20, 2023

@ninsbl I implemented your suggestion to replace getting addons files paths from the GitHub REST API by cloning addons Git repository. The addons_paths.json JSON file is replaced by the addons Git repository directory grass-addons.

@tmszi tmszi requested review from ninsbl and removed request for ninsbl April 20, 2023 06:47
@tmszi
Copy link
Member Author

tmszi commented Apr 21, 2023

@ninsbl my implemented solution doen't work correctly with unofficial addons repository, it will need it fixing.

tmszi added 2 commits April 30, 2023 07:01
To determine which multi-addons install only the manual HTML page.
@tmszi tmszi force-pushed the fix-get-addons-paths-if-file-does-not-exist branch from 69b8f86 to c4411ae Compare May 2, 2023 08:35
Replace using GitHub REST API for getting addons paths

JSON file:
g.extension prefix param arg ($GRASS_ADDON_BASE) dir plus /addons_paths.json
file.

with Git repository

Path:
g.extension prefix param arg ($GRASS_ADDON_BASE) dir plus /grass-addons
dir.
@tmszi tmszi force-pushed the fix-get-addons-paths-if-file-does-not-exist branch from c4411ae to 1665303 Compare May 2, 2023 12:50
@tmszi tmszi dismissed wenzeslaus’s stale review May 3, 2023 07:42

Refactoring to use git instead of GitHub REST API

@tmszi
Copy link
Member Author

tmszi commented May 5, 2023

To be backported with #2895.

@tmszi tmszi merged commit c19cd81 into OSGeo:main May 5, 2023
@tmszi tmszi deleted the fix-get-addons-paths-if-file-does-not-exist branch May 5, 2023 19:02
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
…SGeo#2717)

Replace using GitHub REST API for getting addons paths

JSON file:
g.extension prefix param arg (default $GRASS_ADDON_BASE) dir plus 
/addons_paths.json file.

with Git addons repository

Path:
g.extension prefix param arg (default $GRASS_ADDON_BASE) dir plus 
/grass-addons dir.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CI Continuous integration Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] FileNotFoundError for addons_paths.json
6 participants