Opened 16 months ago

Closed 8 months ago

#3880 closed defect (fixed)

update/drop revision check after git switch

Reported by: hellik Owned by: grass-dev@…
Priority: blocker Milestone: 7.8.3
Component: Compiling Version: svn-releasebranch76
Keywords: svn, git, revision Cc:
CPU: All Platform: All

Description

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.

Change History (23)

comment:1 Changed 16 months ago by neteler

Priority: normalblocker

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.

comment:2 Changed 16 months ago by neteler

See also #3856

comment:4 Changed 15 months ago by martinl

Resolution: fixed
Status: newclosed

comment:5 Changed 11 months ago by neteler

Milestone: 7.6.27.8.3
Resolution: fixed
Status: closedreopened

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

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.

comment:6 Changed 11 months ago by neteler

comment:7 in reply to:  6 ; Changed 11 months ago by 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.

comment:8 in reply to:  7 ; Changed 11 months ago by 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)

comment:9 in reply to:  8 ; Changed 11 months ago by 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?

comment:10 in reply to:  9 Changed 11 months ago by 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.

comment:11 Changed 11 months ago by 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

comment:12 in reply to:  11 ; Changed 11 months ago by 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.

comment:13 in reply to:  12 Changed 11 months ago by 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.

comment:14 Changed 11 months ago by neteler

All backports done and gisinit part of PR278 removed.

comment:15 Changed 11 months ago by neteler

Resolution: fixed
Status: reopenedclosed

comment:16 in reply to:  15 Changed 11 months ago by mmetz

Resolution: fixed
Status: closedreopened

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.

comment:17 Changed 11 months ago by 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.

comment:18 Changed 9 months ago by neteler

comment:19 Changed 8 months ago by neteler

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

comment:20 in reply to:  19 Changed 8 months ago by 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

comment:21 Changed 8 months ago by 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.

comment:22 Changed 8 months ago by 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).

comment:23 in reply to:  21 Changed 8 months ago by martinl

Resolution: fixed
Status: reopenedclosed

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.

Note: See TracTickets for help on using tickets.