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.

Sign up for GitHub

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

Jump to bottom

g.extension: use git sparse-checkout instead of SVN #2895

Merged

ninsbl merged 6 commits into OSGeo:main from ninsbl:g_extension_sparse_checkout

Apr 12, 2023

Member

ninsbl commented Mar 17, 2023

This PR replaces svn export to fetch single addons from the official addon repository with git sparse-checkout. Thus, the dependency on SVN becomes only optional (if SVN repositories are used). See #2834
It also reduces the likelihood that the GitHub API is used (and rate limit is hit) as info about latest changes can be taken from the git logs.
Could be a first step towards also supporting installation of multiple AddOns (or all). Because of possible extented use of the introduced functionality it is implemented as a GitAdapter class.

More cleanup in existing g.extension code should be done (but that is a different PR)...

Leaving the milestone open as I do not know if it is feasible (or appropriate) to get this into 8.3...

ninsbl added 2 commits

March 13, 2023 23:11


          unified imports

634d70d


          use git sparse checkout

c66734c

ninsbl added enhancement Python labels

ninsbl requested review from wenzeslaus and tmszi

March 17, 2023 22:40

Contributor

nilason commented Mar 17, 2023

I’m glad you could make this effort to rework the add-on install! I have not tested nor reviewed the code, but in my opinion, in principle this update should be seriously considered for 8.3. Thanks!

wenzeslaus reviewed

View reviewed changes

Member

wenzeslaus left a comment

I just did a shallow review. General code layout looks good. The Git code promising.

The gscript/grass -> gs is needed, but it makes this PR harder to navigate. It is okay to use it for the new code, i.e., have both import grass.script as gs and import grass.script as gscript.

scripts/g.extension/g.extension.py Outdated

                   return directory
-              def download_source_code_official_github(url, name, outdev, directory=None):
+              def download_source_code_official_github(
+                  url, name, branch, major_grass_version=8, directory=None

Member

wenzeslaus Mar 17, 2023

Try to avoid major_grass_version=8 this would be difficult to maintain.

scripts/g.extension/g.extension.py Outdated

-                          )
-                          % file_url
+                              "Unable to parse '{url}'. Trying to scan"
+                              " git repository (may take some time)..."

Member

wenzeslaus Mar 17, 2023

It is actually a "Git repository" like before it was "Subversion repository".

scripts/g.extension/g.extension.py Outdated

+                      url="https://github.com/osgeo/grass-addons",
+                      working_directory=None,
+                      official_repository_structure=True,
+                      major_grass_version=8,

Member

wenzeslaus Mar 17, 2023

Another place which has the hardcoded major version. See all the desperate commits trying to fix version numbers around every release.

Member Author

ninsbl Mar 19, 2023

Thanks! Yes, that was planned to be set to version extracted at the bottom of the module, forgot to plug that in...

scripts/g.extension/g.extension.py Outdated

Comment on lines 260 to 265

+                      repo_directory = (
+                          self.url.split("/")[-1][0:-4]
+                          if self.url.endswith(".git")
+                          else self.url.split("/")[-1]
+                      )
+                      self.local_copy = self.working_directory / repo_directory

Member

wenzeslaus Mar 17, 2023

If the repo_directory only purpose is to find the directory, you can do it ahead of time. Find the name in the URL, than use the name with git command, then you have it fully under control, can check if it already exists (but the git call can still fail if two g.extensions run in parallel).

Member Author

ninsbl Mar 19, 2023

Right. This is unnecessary complicated. Using a fixed directory name now. Maybe this needs reconsidering when git clone should be used for all git compatible sources...

scripts/g.extension/g.extension.py Outdated

Comment on lines 261 to 263

+                          self.url.split("/")[-1][0:-4]
+                          if self.url.endswith(".git")
+                          else self.url.split("/")[-1]

Member

wenzeslaus Mar 17, 2023

I'm not sure about best practice for splitting URLs (split vs Path vs urlparse), but rsplit with max splits will be better than split.

scripts/g.extension/g.extension.py Outdated

+                      )
+                      branch_list = gs.decode(branch_list.communicate()[0])
+                      return {
+                          branch.split("/")[-1]: branch.split("\t")[0]

Member

wenzeslaus Mar 17, 2023

rsplit

Member Author

ninsbl Mar 19, 2023

Implemented


          address code review

7c0b66a

Member Author

ninsbl commented Mar 19, 2023 •

edited by neteler

Loading

Should address also: #2186
This is however to be considered as another step towards better git support in general.

Maybe a thorough cleanup and refactoring of g.extension would be a good topic for the community sprint...

ninsbl requested a review from wenzeslaus

March 19, 2023 22:31

This was referenced Mar 20, 2023

Problem installing addons #2186

Closed

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

Merged

Contributor

nilason commented Mar 21, 2023

I did test this and installed some addons, seems to work fine. Is there any particular weak spot, that needs testing?

As I mentioned in #2717 (comment) you may try to remove the test_addons_download.py exclusion for Mac CI build.

Contributor

petrasovaa commented Mar 21, 2023

Are there any concerns about the version of git? We need to specify minimum version, no?

Member

echoix commented Mar 21, 2023

In what git version sparse checkout started working like the extension uses? It wasn't possible before, hence why SVN was kept

Contributor

nilason commented Mar 21, 2023

Probably Git 2.27 should be needed (https://stackoverflow.com/a/62480045), sparse-checkout introduced in 2.25 (https://github.com/git/git/blob/e25cabbf6b34e4a6e903d65102d87055cc994778/Documentation/RelNotes/2.25.0.txt#L67)
but fixes in 2.27 may be needed. This is after a very quick check, I may be wrong.

https://repology.org/project/git/versions

Member Author

ninsbl commented Mar 22, 2023

Good points! So, Debian 10 could be problematic and CentOS < 8 will not work either, same for Ubunut 18.04.
I tested with Ubunut 20.04 which has git 2.25.1 and that seems to work fine.

What distros and older versions do we want to support with recent code?
Or do we need a fall-back solution for older systems?

MS Windows is not affected as there svn is not available either and ionly precompiled addons are used...

Member

wenzeslaus commented Mar 22, 2023

...What distros and older versions do we want to support with recent code? Or do we need a fall-back solution for older systems?

I don't have a direct answer, but whatever we want to support 100% should be in the CI (if it is not there, it should be added). The limitations: We are using pre-configured VMs, so the CI environments are not completely same as default installations. We don't test every Linux distro, we just assume that what we test is a good proxy. The recent Ubuntu update in CI was not motivated by version coverage (#2730). CentOS looks like old-system test, but it actually never achieved that; it's a conda on old system test. We don't run tests on CentOS.

We are also still maintaining 7.8 right now if one needs a legacy version.

Contributor

petrasovaa commented Mar 22, 2023

What distros and older versions do we want to support with recent code? Or do we need a fall-back solution for older systems?

Given that GitHub stops SVN support January 8th 2024 (in less than a year) then there is no point in keeping the current svn code. This means also 7.8 g.extension stops working next year. A fallback solution could be to clone the entire repository, no? I think at the very least this PR should check for the version (2.25 minimum I guess) and suggest to update git version instead of failing.

Member Author

ninsbl commented Mar 22, 2023

Thanks for the feedbak! It seems first versions of sparse checkout were introduced in git versions as early as 1.7:
https://github.com/git/git/blob/142430338477d9d1bb25be66267225fb58498d92/Documentation/RelNotes/1.7.0.txt
I found no earlier version than that on repology. Maybe I will try to compile 1.7.0 for a test, and if that works we do not need a fallback...
Should be possible to backport to 7.8 (and 8.2 / 8.0)...

Contributor

nilason commented Mar 22, 2023

Thanks for the feedbak! It seems first versions of sparse checkout were introduced in git versions as early as 1.7:
https://github.com/git/git/blob/142430338477d9d1bb25be66267225fb58498d92/Documentation/RelNotes/1.7.0.txt
I found no earlier version than that on repology. Maybe I will try to compile 1.7.0 for a test, and if that works we do not need a fallback...
Should be possible to backport to 7.8 (and 8.2 / 8.0)...

Strange, considering https://github.blog/2020-01-13-highlights-from-git-2-25/

Contributor

nilason commented Mar 22, 2023

From source above:

Previously, in order to use the sparse-checkout feature you needed to manually edit a file in your .git directory, change a config setting, and run an obscure plumbing command. The instructions for getting back to normal were even more complicated. The new git sparse-checkout command makes this process much easier.

ninsbl added 2 commits

March 24, 2023 23:36


          version dependent fall-back

97b8b33


          test on mac

7dd7890

ninsbl added the backport_needed label

Member Author

ninsbl commented Mar 24, 2023

OK, added a fall-back solution now, that uses a normal checkout for git versions < 2.25.
The fall-back solution is tested in principle locally, but due to the lack of old enough git version, I could not fully test it (meaning if it also works with e.g. git 1.6 or 1.7).

Member Author

ninsbl commented Mar 24, 2023

Here is a comparison of g.extension as of this PR:

time g.extension v.stream.order
WARNING: Extension <v.stream.order> already installed. Re-installing...
Fetching <v.stream.order> from <https://github.com/OSGeo/grass-addons> (be
patient)...
remote: Enumerating objects: 27, done.
remote: Counting objects: 100% (21/21), done.
remote: Compressing objects: 100% (21/21), done.
remote: Total 27 (delta 1), reused 8 (delta 0), pack-reused 6
Receiving objects: 100% (27/27), 67.52 KiB | 2.50 MiB/s, done.
Resolving deltas: 100% (1/1), done.
Already on 'grass8'
Your branch is up to date with 'origin/grass8'.
Compiling...
Installing...
Updating extensions metadata file...
Updating extension modules metadata file...
Installation of <v.stream.order> successfully finished

real	0m12.353s
user	0m4.667s
sys	0m1.339s

and g.extension in GRASS GIS 8.2.1:

time g.extension v.stream.order
WARNING: Extension <v.stream.order> already installed. Re-installing...
Fetching <v.stream.order> from GRASS GIS Addons repository (be patient)...
Compiling...
Installing...
Updating extensions metadata file...
Updating extension modules metadata file...
Installation of <v.stream.order> successfully finished

real	0m10.692s
user	0m1.979s
sys	0m0.601s

By using git, the module becomes a bit more chatty, and takes 20% (or 2 seconds) longer...

A question is if it would be appropriate to get this into 8.3 or if it should be tested a bit more first...
Download tests in CI for Mac are now activated...

Contributor

nilason commented Mar 28, 2023

A question is if it would be appropriate to get this into 8.3 or if it should be tested a bit more first...

Personally I'd welcome it for 8.3. Note, it is RC to come, great opportunity for broader testing!

Download tests in CI for Mac are now activated...

👍

Just a note on the in-line comments on the GitAdapter::__init__ method:
there is a copy-paste typo with "containg", but I'd say most (if not all!) of the comments in that method is superfluous, the code says what need to be said.

Contributor

petrasovaa commented Mar 29, 2023

I tested little bit, I am for 8.3 as well.


          fix typo

c6dcaf5

ninsbl added this to the 8.3.0 milestone

Member Author

ninsbl commented Mar 29, 2023

Thanks for the feedback @nilason and @petrasovaa
I just added this to Milestone 8.3 and fixed the typos, though I would have no objections to remove the comments if they are considered unnecessary...

ninsbl merged commit a7ed30c into OSGeo:main

Member Author

ninsbl commented Apr 12, 2023

Took the liberty to merge this as no RC is out yet and there were no objections to including it into 8.3.
Will wait with backporting until this is a bit more broadly tested...

echoix mentioned this pull request

[Bug] GitHub sunsetting SVN on January 8, 2024 : g.extension and grass impacts #2834

Closed

nilason mentioned this pull request

g.extension: show fatal message if installed Add-On doesn't exists in the repository #2916

Merged

tmszi mentioned this pull request

g.extension: fix printing extension temporary directory to stderr for downloading extension source code only with -d flag #2918

Merged

ninsbl mentioned this pull request

Dynamic subdirs in Makefile OSGeo/grass-addons#887

Merged

This was referenced Apr 19, 2023

g.extension: refactor/simplify 'GitAdapter' class '_get_addons_list' method #2925

Merged

g.extension: fix extracting Git version string #2929

Merged

tmszi mentioned this pull request

g.extension: fix addon SOURCE_URL env var value for official GitHub addons #2936

Merged

neteler mentioned this pull request

contributing: Add instructions on unit tests OSGeo/grass-addons#916

Merged

landam removed the backport_needed label

neteler pushed a commit to nilason/grass that referenced this pull request


          g.extension: use git sparse-checkout instead of SVN (OSGeo#2895)

4bfc3c2

* unified imports
* use git sparse checkout
* version dependent fall-back
* test on mac

wenzeslaus mentioned this pull request

temporal: handle SQLite < 3.33 #3723

Merged

neteler mentioned this pull request

[Bug] g.extension uses wrong path OSGeo/grass-addons#1089

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment