g.extension: get branch from version #1700

ninsbl · 2021-07-01T22:18:20Z

Replaces #1678
Once we have a grass8 branch in grass-addons, the hardcoded version 7 should be removed from g.extension...

ninsbl · 2021-07-02T08:06:00Z

It would be nice if we could remove the hardcoded version here:

grass/scripts/g.extension/g.extension.py

Lines 2508 to 2511 in 3d86470

    
           # TODO: update temporary workaround of using grass7 subdir of addon-repo, see 
        
           #       https://github.com/OSGeo/grass-addons/issues/528 
        
           version[0] = 7 
        
           version[1] = 9

Then we do not have to change that in master later on and it probably causes less trouble with addons on windows where we have pre-compiled addons going back to version 7.4...

But that requires a branch "grass8" in the addon repo for g.extension to work with GRASS 8 (or current master)...

wenzeslaus · 2021-07-05T03:29:42Z

scripts/g.extension/g.extension.py

-        if branch in ["master", "main"]:
-            svn_reference = "trunk"
+        if branch is None or branch in ["master", "main"]:
+            svn_reference = "branches/grass{}".format(version)


This is confusing. None (default) may be expected to resolve to the default branch which is represented by trunk in Subversion interface, although perhaps default resulting in the version branch makes sense in this context. However, explicit master or main should resolve to branch/master and branch/main unless there is a clear reason to do it otherwise.

I though -o is somewhat "behave as with the official repository" which is broader than its defintion "url refers to a fork of the official extension repository". Also depends on what people think fork is. Overall, this might be limiting when you let's say create an institutional repo which should work as the official repository, but you support only one version.

scripts/g.extension/g.extension.py

wenzeslaus · 2021-07-05T03:38:23Z

But that requires a branch "grass8" in the addon repo for g.extension to work with GRASS 8 (or current master)...

To maximize freshness of addons for v7 and minimize the backporting needs, it would be best to have a fix in code for v8 and create grass8 branch only close to the release (perhaps we are already?). The v8 specific fix can be later removed or rewritten for v9 because the idea now is that we just do the same grassX to grassY branch switch for v8->v9 as we will do for v7->v8.

ninsbl · 2021-07-05T10:10:47Z

But that requires a branch "grass8" in the addon repo for g.extension to work with GRASS 8 (or current master)...

To maximize freshness of addons for v7 and minimize the backporting needs, it would be best to have a fix in code for v8 and create grass8 branch only close to the release (perhaps we are already?). The v8 specific fix can be later removed or rewritten for v9 because the idea now is that we just do the same grassX to grassY branch switch for v8->v9 as we will do for v7->v8.

In 1b39a61 I tried to solve this by listing branches via github API (should be OK, if addons are supposed to be fetched from online repo).
The logic is to pick the branch of the current version if present in the repo (and if no branch is specified explicitly), otherwise take the latest branch. That should work for grass8 and 9 and hardcoded versions are not necessary, unless you would want to fetch addons for e.g. GRASS 7.8. from grass8 branch by default...

neteler · 2021-07-05T19:44:53Z

(for an easier backporting, g.extensionin the G78 branch needs to receive a black update first)

scripts/g.extension/g.extension.py

ninsbl · 2021-07-05T22:32:08Z

Thanks for your feedback!

scripts/g.extension/g.extension.py

ninsbl · 2021-07-19T22:40:22Z

Should be ready to merge.
I fixed the issues from review (and some additional) and the download tests pass locally.
Given the blocker status, maybe best to merge now, and then backport all required changes of g.extension to the release branch. We should probably check if it would work with 7.6 as well...

wenzeslaus

We can merge this as is. I have some additional comments which don't need to be addressed in this PR. I marked most of the conversations as resolved.

wenzeslaus · 2021-07-20T01:11:38Z

scripts/g.extension/g.extension.py

+    # Parse URL
+    url_parts = urlparse(full_url)
+    # Get organization and repository component
+    try:
+        organization, repository = url_parts.path.split("/")[
+            1:3
        ]
+    except URLError:
+        gscript.fatal(_("Cannot retrieve organization and repository from URL: <{}>.".format(full_url)))


Having this part here (and not e.g. in a separate, possibly nested function and called only when needed), prohibits general URLs to be used here (just to get main). I'm not sure how relevant that is, though.

This function is only meant for known hosting services (currently: github, gitlab, bitbucket), so, this should not be an issue...

I was thinking the main would be used as a the default for any other service, but in that case, use likely needs to (and can) specify branch explicitly anyway.

wenzeslaus · 2021-07-20T01:20:30Z

scripts/g.extension/g.extension.py

+        repo_branches = get_github_branches()
+        # Define branch to fetch from (latest or current version)
+        version_branch = "grass{}".format(version[0])
+        if version_branch not in repo_branches:
+            version_branch = repo_branches[-1]


Agreed that documentaion is needed for that, but maybe in another PR. The whole "fork" think here is suboptimal. Typical (soft) fork on GitHub will have the branches. Do we mean hard project fork? I think the aim is institutional repositories (private or public), right? That was the case in the past, I think. I'm not sure how to call "a repo with the same structure as the official repo", plus there is the issues with the generated metadata file which is separate if I recall correctly. ...next PR. Perhaps open an issue?

wenzeslaus · 2021-07-20T01:27:56Z

scripts/g.extension/g.extension.py

+                get_default_branch(actual_start)
+                if urlparse(actual_start).netloc == "gitlab.com"
+                else "main"


Isn't this what get_default_branch is doing?

Yes. I moved it into the function in the latest commit.

As for the "fork" issue, I agree that it should be more clear.
The case I had in mind was an addon-dev contributing a new addon, for example. Until the fork-flag got implemented, it was not possible to test contributions to the official addon repo without cloning the repo of the addon-dev and installing from local URL. So, the main intention has been to allow to install directly from e.g. a feature branch of a (soft) fork of the official repo...
Should be probably added to the manual....

As for the "fork" issue...The case I had in mind was an addon-dev contributing a new addon...

Just for the record, I think in the past (Subversion times) it was possible to install from repositories which behaved like the official repo (I think @neteler or @landam created some). Today, the "test from contributor's fork" use case you mentioned is definitively more important, esp. since individual modules can be installed from small repos (without a need for mirroring the official repo infrastructure).

wenzeslaus · 2021-07-20T02:12:15Z

This can be tested without compiling or download using Binder:

wenzeslaus · 2021-07-21T03:45:13Z

@ninsbl Leaving the fork-related conversations open (I marked the rest as resolved). Please, resolve them as you see fit. Thank you.

ninsbl · 2021-07-21T14:43:06Z

Thanks, @wenzeslaus for bearing with me here. I mentioned this PR in the ticket about general git related improvements of g.extension. Refactoring g.extension may be a good task for the next community sprint...

But an open question to me is how we backport best? Not all earlier changes in g.extension were backported, see:
https://github.com/OSGeo/grass/commits/master/scripts/g.extension/g.extension.py
vs.
https://github.com/OSGeo/grass/commits/releasebranch_7_8/scripts/g.extension/g.extension.py
Should the version from master be copied into the releasebranch? And should we try to support the new addon-repo also for GRASS 7.6 (and Python 2), as that is still the most recent version on some systems?

neteler · 2021-07-21T20:36:15Z

Thanks for your hard work!

Let me suggest to backport it completely to 7.8 and create a new 7.8.6 RC2 to get it tested.

Concerning 7.6, I don't really expect a new release. They may upgrade to at least 7.8 - see also https://repology.org/project/grass/versions.

neteler · 2021-12-05T09:00:30Z

Update: the grass8 branch has recently been created in grass-addons (new default branch).

Shall this PR be backported now to G7.8.7? Would any changes be needed for that (i.e., in new PR), @ninsbl?

neteler · 2022-01-08T07:17:06Z

Gentle ping, @ninsbl :)

neteler · 2022-01-08T07:59:13Z

Ah, if I understand correctly, it has already been backported in #1762 (?)

wenzeslaus · 2022-05-16T21:21:10Z

This is in 8.0 and 8.2 branches and seems to be in 7.8 as 42f015e (#1700), so removing the backport label.

* branch from version * black * address review, flake8 * get branch from github API * handle branch=None * handle branch=None better * black/flake8 * cleanup after code review * update from code review * try orgs and users * use repo API * pass propper API URL * defaults for get_github_branches, fix l-flag, updates from code review * abandon github API due to rate limits * version over default * use URLError * use urlparse * use urlparse * use get_default_branch for gitlab * fix keyword issue with urlopen * global GIT_URL, get_default update

branch from version

1d3e709

ninsbl added the enhancement New feature or request label Jul 1, 2021

ninsbl added this to the 7.8.6 milestone Jul 1, 2021

ninsbl marked this pull request as draft July 1, 2021 22:18

black

a4f3c8a

neteler added backport_needed blocker Blocking a release labels Jul 2, 2021

wenzeslaus requested changes Jul 5, 2021

View reviewed changes

ninsbl added 4 commits July 5, 2021 09:39

address review, flake8

336daa7

get branch from github API

1b39a61

handle branch=None

6d73c2f

handle branch=None better

45d80b8

black/flake8

d221d31

ninsbl requested a review from wenzeslaus July 5, 2021 10:16

wenzeslaus requested changes Jul 5, 2021

View reviewed changes

cleanup after code review

ca14583

ninsbl requested a review from wenzeslaus July 5, 2021 22:31

wenzeslaus requested changes Jul 6, 2021

View reviewed changes

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

ninsbl added 3 commits July 6, 2021 10:34

update from code review

59ab627

try orgs and users

cdfa398

use repo API

dd9d368

wenzeslaus requested changes Jul 7, 2021

View reviewed changes

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

scripts/g.extension/g.extension.py Show resolved Hide resolved

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

wenzeslaus mentioned this pull request Jul 8, 2021

Update to upstream ctypesgen version #1651

Merged

veroandreo mentioned this pull request Jul 8, 2021

i.superpixel.slic extension not found OSGeo/grass-addons#581

Closed

pass propper API URL

7e479bc

version over default

c906308

wenzeslaus requested changes Jul 12, 2021

View reviewed changes

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

scripts/g.extension/g.extension.py Outdated Show resolved Hide resolved

ninsbl added 5 commits July 19, 2021 23:38

use URLError

9a5829f

use urlparse

290cd96

use urlparse

6853784

use get_default_branch for gitlab

fab3787

fix keyword issue with urlopen

9bb3b87

ninsbl requested a review from wenzeslaus July 19, 2021 22:36

ninsbl marked this pull request as ready for review July 19, 2021 22:37

wenzeslaus approved these changes Jul 20, 2021

View reviewed changes

global GIT_URL, get_default update

866d18f

wenzeslaus approved these changes Jul 21, 2021

View reviewed changes

ninsbl mentioned this pull request Jul 21, 2021

[Feat] g.extension: improved git support #625

Open

11 tasks

ninsbl merged commit 4705cc5 into OSGeo:master Jul 21, 2021

ninsbl deleted the g_extension_version branch July 21, 2021 14:33

wenzeslaus mentioned this pull request Aug 24, 2021

Restructure repository for version 8 OSGeo/grass-addons#528

Closed

17 tasks

landam modified the milestones: 7.8.6, 7.8.7 Oct 7, 2021

wenzeslaus removed the backport_needed label May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

g.extension: get branch from version #1700

g.extension: get branch from version #1700

ninsbl commented Jul 1, 2021

ninsbl commented Jul 2, 2021

wenzeslaus Jul 5, 2021

wenzeslaus commented Jul 5, 2021

ninsbl commented Jul 5, 2021

neteler commented Jul 5, 2021

ninsbl commented Jul 5, 2021

ninsbl commented Jul 19, 2021

wenzeslaus left a comment

wenzeslaus Jul 20, 2021

ninsbl Jul 20, 2021

wenzeslaus Jul 21, 2021

wenzeslaus Jul 20, 2021

wenzeslaus Jul 20, 2021

ninsbl Jul 20, 2021

ninsbl Jul 20, 2021

wenzeslaus Jul 21, 2021

wenzeslaus commented Jul 20, 2021

wenzeslaus commented Jul 21, 2021

ninsbl commented Jul 21, 2021

neteler commented Jul 21, 2021 •

edited

Loading

neteler commented Dec 5, 2021 •

edited

Loading

neteler commented Jan 8, 2022

neteler commented Jan 8, 2022

wenzeslaus commented May 16, 2022

g.extension: get branch from version #1700

g.extension: get branch from version #1700

Conversation

ninsbl commented Jul 1, 2021

ninsbl commented Jul 2, 2021

Choose a reason for hiding this comment

wenzeslaus commented Jul 5, 2021

ninsbl commented Jul 5, 2021

neteler commented Jul 5, 2021

ninsbl commented Jul 5, 2021

ninsbl commented Jul 19, 2021

wenzeslaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenzeslaus commented Jul 20, 2021

wenzeslaus commented Jul 21, 2021

ninsbl commented Jul 21, 2021

neteler commented Jul 21, 2021 • edited Loading

neteler commented Dec 5, 2021 • edited Loading

neteler commented Jan 8, 2022

neteler commented Jan 8, 2022

wenzeslaus commented May 16, 2022

neteler commented Jul 21, 2021 •

edited

Loading

neteler commented Dec 5, 2021 •

edited

Loading