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

grass.py: used space as delimiter to get GRASS_VERSION_STRING #405

Merged
merged 13 commits into from
Apr 20, 2020

Conversation

ninsbl
Copy link
Member

@ninsbl ninsbl commented Mar 6, 2020

grass --config currently throws an error:

Traceback (most recent call last):
  File "/usr/local/Cellar/osgeo-grass/7.8.2_3/libexec/bin/grass78", line 2025, in main
    index = sys.argv.index(batch_exec_param)
ValueError: '--exec' is not in list

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/Cellar/osgeo-grass/7.8.2_3/libexec/bin/grass78", line 2216, in <module>
    main()
  File "/usr/local/Cellar/osgeo-grass/7.8.2_3/libexec/bin/grass78", line 2030, in main
    params = parse_cmdline(sys.argv[1:], default_gui=default_gui)
  File "/usr/local/Cellar/osgeo-grass/7.8.2_3/libexec/bin/grass78", line 1951, in parse_cmdline
    print_params()
  File "/usr/local/Cellar/osgeo-grass/7.8.2_3/libexec/bin/grass78", line 1862, in print_params
    "%s\n" % val[0].split(':')[1].rstrip('$"\n').strip())
IndexError: list index out of range

This is very inconvenient if you want to determine where GRASS is installed e.g. for usage in R or the like.

The reason for the error is that colon is used as delimiter to parse GIS_H_VERSION and GRASS_VERSION_STRING.

Unfortunately, GRASS_VERSION_STRING is not replaced with the proper value and I have no idea ho to do that...

@ninsbl ninsbl requested review from wenzeslaus and metzm March 6, 2020 09:37
@ninsbl ninsbl added bug Something isn't working backport_needed labels Mar 6, 2020
@ninsbl ninsbl added this to the 7.8.3 milestone Mar 6, 2020
@ninsbl
Copy link
Member Author

ninsbl commented Mar 6, 2020

BTW: applying #325 does not provide revision info in:
grass --config

@metzm
Copy link
Contributor

metzm commented Mar 6, 2020

Looks good to me, but it seems that this PR requires PR #325. Therefore it can only be merged after #325 has been merged, correct?

@ninsbl
Copy link
Member Author

ninsbl commented Mar 6, 2020

This has been independent from #325 until now.
With the latest commit however, the GRASS_HEADERS_VERSION is taken from version.h by grass --config and (new) also GRASS_HEADERS_DATE (so #325 needs to be merged first now).

The GRASS_VERSION_STRING in version.h is currently (with #325 applied) @(#) 7.9.dev (2020), while GRASS_HEADERS_VERSION is 7.9.dev (2020) fdae3b2441. Note sure if GRASS_VERSION_STRINGshould be reported instead of GRASS_HEADERS_DATE. It seems though the former should be modified in #325...

@metzm
Copy link
Contributor

metzm commented Mar 8, 2020

This has been independent from #325 until now.

I see, because git is not expanding the sv keyword Revision.

With the latest commit however, the GRASS_HEADERS_VERSION is taken from version.h by grass --config and (new) also GRASS_HEADERS_DATE (so #325 needs to be merged first now).

I don't think these are the intended values for version and date, instead grass.py probably wants to report the latest git hash of the whole source tree, not only the headers. Instead of using GRASS_HEADRS_VERSION from version.h, we could insert GRASS_VERSION_GIT from Platform.make when creating grass.py, i.e. modify lib/init/Makefile at the lines following L68:
https://github.com/OSGeo/grass/blob/master/lib/init/Makefile#L68

@ninsbl
Copy link
Member Author

ninsbl commented Mar 8, 2020

You are right, of course. Now I tried to address that in the two new commits.

@ninsbl
Copy link
Member Author

ninsbl commented Mar 11, 2020

Sorry, @wenzeslaus, I saw #323 just now, which obviously adresses the same issue, though solves it slightly different...
Feel free to disregard (close) this one…
But it would be great to get either this or #323 into 7.8.3...

@metzm
Copy link
Contributor

metzm commented Mar 11, 2020

@ninsbl @wenzeslaus The question is what exactly grass7x --config revision should report. The git hash of the latest commit to this branch, or the git hash of the latest commit to /include of this branch?

For grass7x --config revision, I prefer the git hash of the latest commit to this branch, not only to /include of this branch.

@ninsbl
Copy link
Member Author

ninsbl commented Mar 19, 2020

Should fix:
https://trac.osgeo.org/grass/ticket/4001

@wenzeslaus
Copy link
Member

@ninsbl Feel free to use pieces of #323 if they seem useful. Maybe there are two issues to solve: 1) robust parsing (to e.g. gracefully fail, try harder to parse, etc., kind of what I tried in #323) and 2) Actually parse the right/current value (e.g. after #325).

I second @metzm in saying that we need to clarify what this should actually report. Perhaps it is a topic for different PR/issue. See, for example:

$ # 7.6.1 installed from PPA
$ grass76 --config version
7.6.1
$ grass76 --config revision
72327
$ grass76 --config svn_revision
xported

$ # 7.9 from #323 
$ grass79 --config version
7.9.dev
$ grass79 --config revision
7.9.dev (2020)
$ grass79 --config svn_revision
2f4104

@neteler
Copy link
Member

neteler commented Apr 1, 2020

I tested this PR (since PR #325 has been merged), and this is the result

grass79 --config
x86_64-pc-linux-gnu
./configure  --with-cxx --enable-largefile --with-proj --with-proj-share=/usr/share/proj --with-gdal=/usr/bin/gdal-config --with-python --with-geos --with-sqlite --with-nls --with-zstd --with-pdal --with-cairo --with-cairo-ldflags=-lfontconfig --with-freetype --with-freetype-includes=/usr/include/freetype2 --with-wxwidgets --with-fftw --with-postgres --with-postgres-includes=/usr/include/pgsql --with-blas --with-blas-includes=/usr/include/atlas-x86_64-base/ --with-lapack --with-lapack-includes=/usr/include/atlas-x86_64-base/ --with-netcdf --without-motif --without-bzlib --without-mysql --without-odbc --without-openmp --without-ffmpeg
gcc
/home/mneteler/software/grass_master/dist.x86_64-pc-linux-gnu
f062bffc8
7.9.dev

... no more error.

And here with the available parameters:

for i in arch build compiler path revision svn_revision version date ; do 
   echo "" ; echo "Testing --config $i:" ; grass79 --config $i
done

Testing --config arch:
x86_64-pc-linux-gnu

Testing --config build:
./configure  --with-cxx --enable-largefile --with-proj --with-proj-share=/usr/share/proj --with-gdal=/usr/bin/gdal-config --with-python --with-geos --with-sqlite --with-nls --with-zstd --with-pdal --with-cairo --with-cairo-ldflags=-lfontconfig --with-freetype --with-freetype-includes=/usr/include/freetype2 --with-wxwidgets --with-fftw --with-postgres --with-postgres-includes=/usr/include/pgsql --with-blas --with-blas-includes=/usr/include/atlas-x86_64-base/ --with-lapack --with-lapack-includes=/usr/include/atlas-x86_64-base/ --with-netcdf --without-motif --without-bzlib --without-mysql --without-odbc --without-openmp --without-ffmpeg

Testing --config compiler:
gcc

Testing --config path:
/home/mneteler/software/grass_master/dist.x86_64-pc-linux-gnu

Testing --config revision:
f062bffc8

Testing --config svn_revision:
062bffc8

Testing --config version:
7.9.dev

Testing --config date:
Parameter <date> not supported

Maybe -config date needs to be aligned to the merged PR #325? (I didn't check the code)

@ninsbl
Copy link
Member Author

ninsbl commented Apr 7, 2020

OK, will update the PR to cover changes in #325, hopefully tonight. Would be nice to get this into 7.8.3...

@neteler
Copy link
Member

neteler commented Apr 18, 2020

@ninsbl shall this be included in 7.8.3 yet?

@landam
Copy link
Member

landam commented Apr 19, 2020

The format of a date could be probably beatified (no indent, no quotes)

./bin.x86_64-pc-linux-gnu/grass79 --config date
    "2020-03-31T20:34:57+02:00"

@landam landam closed this Apr 19, 2020
@landam landam reopened this Apr 19, 2020
@landam
Copy link
Member

landam commented Apr 19, 2020

The format of a date could be probably beatified (no indent, no quotes)

./bin.x86_64-pc-linux-gnu/grass79 --config date
    "2020-03-31T20:34:57+02:00"

Improved in 317631e

@neteler
Copy link
Member

neteler commented Apr 19, 2020

I tested this PR again with the available parameters:

for i in arch build compiler path revision svn_revision version date ; do 
   echo "" ; echo "Testing --config $i:" ; grass79 --config $i
done

Testing --config arch:
x86_64-pc-linux-gnu

Testing --config build:
./configure  --with-cxx --enable-largefile --with-proj --with-proj-share=/usr/share/proj --with-gdal=/usr/bin/gdal-config --with-python --with-geos --with-sqlite --with-nls --with-zstd --with-pdal --with-cairo --with-cairo-ldflags=-lfontconfig --with-freetype --with-freetype-includes=/usr/include/freetype2 --with-wxwidgets --with-fftw --with-postgres --with-postgres-includes=/usr/include/pgsql --with-blas --with-blas-includes=/usr/include/atlas-x86_64-base/ --with-lapack --with-lapack-includes=/usr/include/atlas-x86_64-base/ --with-netcdf --without-motif --without-bzlib --without-mysql --without-odbc --without-openmp --without-ffmpeg

Testing --config compiler:
gcc

Testing --config path:
/home/mneteler/software/grass_master/dist.x86_64-pc-linux-gnu

Testing --config revision:
745ee7ec9

Testing --config svn_revision:
062bffc8

Testing --config version:
7.9.dev

Testing --config date:
Tue Mar 31 20:34:57 2020 +0200

Looks now all good to me.

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.

What is here is fine, but it seems that all these --config values need documentation.

lib/init/grass.py Outdated Show resolved Hide resolved
@hellik
Copy link
Member

hellik commented Apr 20, 2020

Testing --config revision:
745ee7e

Testing --config svn_revision:
062bffc8

Looks now all good to me.

should svn_revision be renamed to be something more git related as svn isn't used anymore?

@landam
Copy link
Member

landam commented Apr 20, 2020

should svn_revision be renamed to be something more git related as svn isn't used anymore?

yes, eg. libgis_revision (?) But keep svn_revision for backward compatibility.

Co-Authored-By: Markus Neteler <neteler@gmail.com>
@ninsbl ninsbl merged commit f59a1c0 into OSGeo:master Apr 20, 2020
landam added a commit that referenced this pull request Apr 20, 2020
* use space delimiter for GIS_H_VERSION

* choose GRASS_VERSION_STRING list element

* report header version and date

* write GRASS_VERSION_GIT to grass.py

* use GRASS_VERSION_GIT

* write GRASS_VERSION_GIT to grass.py

* fix tab indent

* add date

* add date

* get date correctly after #325

* get date correctly after #325

* date: no indent, no quotes

* Update lib/init/grass.py

Co-Authored-By: Markus Neteler <neteler@gmail.com>

Co-authored-by: Stefan Blumentrath <stefan@vm-srv-wallace.vm.ntnu.no>
Co-authored-by: Martin Landa <landa.martin@gmail.com>
Co-authored-by: Markus Neteler <neteler@gmail.com>
@ninsbl
Copy link
Member Author

ninsbl commented Apr 20, 2020

Backported in ea2dd9e
I hope I did not make a mess when backporting...

@landam
Copy link
Member

landam commented Apr 20, 2020

No mess, thanks for PR!

neteler added a commit that referenced this pull request Apr 26, 2020
* grass7 manual: document --config parameters

Adding missing `grass --config <option>` documentation

Adresses #405 (review)

* fix double white space

* fix version for consistency
neteler added a commit that referenced this pull request Apr 26, 2020
* grass7 manual: document --config parameters

Adding missing `grass --config <option>` documentation

Adresses #405 (review)

* fix double white space

* fix version for consistency
@ninsbl ninsbl deleted the grassinit branch June 24, 2020 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants