update/drop revision check after git switch

[hellik]

see ​gh comment

Yeah, the revision check no longer works. I suppose it needs to be dropped since git does not provide anything similar (AFAIK).

the issue popped up while adapting the precompiled winGRASS addon system.

[neteler]

The code in question is here:

# include/gis.h
41:#define GIS_H_VERSION "$Revision$"
44:#define G_gisinit(pgm) G__gisinit(GIS_H_VERSION, (pgm))
45:#define G_no_gisinit() G__no_gisinit(GIS_H_VERSION)


# lib/gis/gisinit.c, line 86ff
    if (strcmp(version, GIS_H_VERSION) != 0)
        G_fatal_error(_("Module built against version %s but "
                        "trying to use version %s. "
                        "You need to rebuild GRASS GIS or untangle multiple installations."),
                        version, GIS_H_VERSION);
    gisinit();

This needs urgently to be addressed since SVN "$Revision$" doesn't exist any more in git.

[neteler]

See also #3856

[neteler]

PR available at ​https://github.com/OSGeo/grass/pull/117

[martinl]

Closed by ​https://github.com/OSGeo/grass/pull/117

[neteler]

The changes in ​https://github.com/OSGeo/grass/pull/117 were reverted yesterday in

• master: ​https://github.com/OSGeo/grass/commit/6617ffb4edd9ffcf1cd016958f70a93dc9229e15
• relbr78: ​https://github.com/OSGeo/grass/commit/156cdb5c3b91db2dd4183247b36b724c419db574

However, I am not convinced that it should be reverted. Rationale: The old SVN tag

​https://github.com/OSGeo/grass/blob/8b9512b8b2fb0531df848fce58619308cbc29b3d/include/gis.h#L41

is no longer expanded after the migration to git and in the last many months we didn't find a git equivalent (as it simply doesn't exist).

Reverting is fine once an alternative has been found. For now we have the risk that addons are broken again.

[neteler]

Note: it lead to ​https://github.com/OSGeo/grass/pull/278

[mmetz]

Replying to neteler:

Note: it lead to ​https://github.com/OSGeo/grass/pull/278

The patch in PR278 for gisinit.c is masking a more serious problem with ctypes generated for Python 3.8: the string "$Revision$" is truncated to its first character "$", then "$" is compared to "$Revision$" which fails of course. Such truncations of strings to their first character must occur elsewhere too, which will only become apparent at a later stage when executing a Python module that uses ctypes, probably causing fairly cryptic errors.

The test in gisinit.c as it is now always compares "$Revision$" to "$Revision$" (in GRASS C modules) which always succeeds which is bad. We could instead use GRASS_VERSION_STRING from include/version.h to catch the obvious cases: module x as been compiled against GRASS 7.y and is attempted to be used with GRASS 7.z.

[neteler]

Replying to mmetz:

We could instead use GRASS_VERSION_STRING from include/version.h to catch the obvious cases: module x as been compiled against GRASS 7.y and is attempted to be used with GRASS 7.z.

This sounds good to me.

(of course the "docker/alpine/alpine-py38-gisinit.patch" in ​https://github.com/OSGeo/grass/pull/278 can then be removed)

[mmetz]

Replying to neteler:

Replying to mmetz:

We could instead use GRASS_VERSION_STRING from include/version.h to catch the obvious cases: module x as been compiled against GRASS 7.y and is attempted to be used with GRASS 7.z.

This sounds good to me.

OK

(of course the "docker/alpine/alpine-py38-gisinit.patch" in ​https://github.com/OSGeo/grass/pull/278 can then be removed)

This patch does not solve problems, it masks problems because no errors occur in GRASS core, only when compiling certain addons.

The patch in PR278 for gisinit.c is masking a more serious problem with ctypes generated for Python 3.8: the string "$Revision$" is truncated to its first character "$", then "$" is compared to "$Revision$" which fails of course. Such truncations of strings to their first character must occur elsewhere too, which will only become apparent at a later stage when executing a Python module that uses ctypes, probably causing fairly cryptic errors.

Which parts of this description need to be elaborated?

[neteler]

Replying to mmetz:

The patch in PR278 for gisinit.c is masking a more serious problem with ctypes

Keep in mind that this temporary patch is only in the Alpine build.

Which parts of this description need to be elaborated?

All fine - the question (for me) is only how to change the gisinit.c code to use GRASS_VERSION_STRING.

[hellik]

the original report was opened because winGRASS addons were broken after switch to git.

The changes in ​​https://github.com/OSGeo/grass/pull/117 were reverted yesterday in

the addons will be broken again with the reverted changes until another solution will be found

[mmetz]

Replying to hellik:

the original report was opened because winGRASS addons were broken after switch to git.

The changes in ​​https://github.com/OSGeo/grass/pull/117 were reverted yesterday in

the addons will be broken again with the reverted changes until another solution will be found

I have replaced the SVN keyword $Revision$ with the generic GRASS_VERSION_STRING in master ​ec9b6f2.

The confusion of main GRASS checked out with git and addons checked out with svn should be solved by now.

[neteler]

Replying to mmetz:

I have replaced the SVN keyword $Revision$ with the generic GRASS_VERSION_STRING in master ​ec9b6f2.

Thanks!

Important: make distclean is needed to compile GRASS GIS.

The confusion of main GRASS checked out with git and addons checked out with svn should be solved by now.

This needs to be backported, probably down to 7.4. I can do that.

[neteler]

All backports done and gisinit part of PR278 removed.

[mmetz]

Re-opening the ticket because g.version needs to be updated as well to avoid

WARNING: GRASS GIS libgis version and date number not available

Also, let's wait for confirmation from wingrass addon building that this is still working.

[mmetz]

Regarding version and date check as in ​lib/gis G__gisinit(), ​lib/gis G__no_gisinit() and ​g.version, the hash and date of the last git commit can be easily obtained for any file/directory and used to define ​GIS_H_VERSION and ​GIS_H_DATE.

The hash and date of the latest commit to the include folder can be obtained with e.g.

git log -1 --pretty=format:"%H|%cd" -- include

catching any changes in the GRASS header files.

This git commit hash and date can be stored in a file and then used by configure to update include/version.h after this header has been generated (also by configure).

Obviously, configure must also work if git is not available or if the source code is not a git clone.

The challenge is to automatically update the file with the hash and date of the latest git commit to the include folder. Updating and uploading this file will require another commit. We could provide a script to update the file with the hash and date of the latest git commit to the include folder and add a ​git hook to make sure this hash+date file has been updated if any changes have been made to the include folder.

The challenge is to create an appropriate ​git hook.

[neteler]

For a fix, see PR325 (​https://github.com/OSGeo/grass/pull/325)

[neteler]

The updated PR325 (​​https://github.com/OSGeo/grass/pull/325) has been merged today, yet a backport is needed (or modification).

[hellik]

Replying to neteler:

The updated PR325 (​​https://github.com/OSGeo/grass/pull/325) has been merged today, yet a backport is needed (or modification).

lets test the upcoming daily builds

[neteler]

This is how it looks now:

GRASS 7.9.dev (nc_spm_08):~/software/grass_master > g.version -rge
version=7.9.dev
date=2020
revision=f062bffc8
build_date=2020-03-31
build_platform=x86_64-pc-linux-gnu
build_off_t_size=8
libgis_revision=f062bffc8
libgis_date="Tue Mar 31 20:34:57 2020 +0200"
proj=5.2.0
gdal=2.3.2
geos=3.7.1
sqlite=3.30.0

and

GRASS 7.9.dev (nc_spm_08):~ > g.extension i.sentinel
WARNING: Extension <i.sentinel> already installed. Re-installing...
Fetching <i.sentinel> from GRASS GIS Addons repository (be patient)...
Compiling...
Installing...
Updating addons metadata file...
Updating private addons metadata file...
WARNING: No addons metadata available. Addons metadata file not updated.  <<---- weird?
Installation of <i.sentinel> successfully finished

The addons metadata warning is a bit weird.

[nila]

My guess is that the issue of "double man1 in path" #3818 is the cause of this. The metadata file went (wrongly) to a man1/man1 directory which cannot be found now after the fix (​https://github.com/OSGeo/grass-addons/pull/93).

[martinl]

Replying to neteler:

> WARNING: No addons metadata available. Addons metadata file not updated.  <<---- weird?

There is something strange with i.sentinel. Not related to original report.

I am taking liberty to close this issue. Feel free to reopen if needed.