search

landam / grass-gis-git-migration-test

0 stars 0 forks source link

Remove legacy meaning of LOCATION variable #169

Open landam opened 5 years ago

landam commented 5 years ago

Reported by wenzeslaus on 19 May 2015 22:15 UTC Especially in older and stable code like grass.py we still keep the legacy LOCATION variable with meaning "full path to a Mapset".

The bug https://trac.osgeo.org/grass/ticket/2679 suggests that nobody is actually using it. So, there is no need for keeping this for backward compatibility anymore.

I suggest to gradually change the meaning to "Location name" and use different variable name for "full path to a Mapset". This can be a gradual change with keeping some compatibility or providing warnings. For example, the "gisrc" file can support both LOCATION and LOCATION_NAME as "Location name".

Dropping of LOCATION as "full path to a Mapset" can be without consequences but replacing LOCATION_NAME by LOCATION might be possible only for GRASS GIS 8 because of usages outside GRASS by users or in other projects (this might be valid only for the "gisrc" file):

In source code, LOCATION and location variables with meaning "full Mapset path" can be replaced by mapset_path. (I've also tried full_mapset but it doesn't seem nice.)

In the "gisrc" file LOCATION_NAME would be replaced by LOCATION.

In environment variables interface, LOCATION_NAME would be replaced by LOCATION and original LOCATION by MAPSET_PATH.

It is questionable if we want to support input environment variable with meaning "full Mapset path" when we already support setting Location and Mapset separately. The "gisrc" file does not support it anyway.

See also https://trac.osgeo.org/grass/ticket/2679 for possible removal or restoration of environment variables for setting Location and Mapset.

Setting as blocker for 8.0.0 because this can be fully resolved only with new version but there are changes which could be done now such as (only) dropping the LOCATION environmental variable as input.

GRASS GIS version and provenance

svn-trunk

Migrated-From: https://trac.osgeo.org/grass/ticket/2681

landam commented 5 years ago

Comment by glynn on 20 May 2015 12:45 UTC Replying to [ticket:2681 wenzeslaus]:

Especially in older and stable code like grass.py we still keep the legacy LOCATION variable with meaning "full path to a Mapset".

The bug https://trac.osgeo.org/grass/ticket/2679 suggests that nobody is actually using it. So, there is no need for keeping this for backward compatibility anymore.

Originally, GISDBASE, LOCATION_NAME and MAPSET were actual environment variables. Even after G_getenv() etc were added, the startup script continued to set the environment variables for the benefit of scripts. When g.mapset was added, we had to change scripts to explicitly query the environment using g.gisenv, because g.mapset cannot change the environment variables of its parent process (e.g. the shell). To ensure that this was done, the startup script stopped setting the environment variables (https://trac.osgeo.org/grass/changeset/10655), so that unfixed scripts failed rather than using a possibly-incorrect environment. The export for LOCATION was re-added temporarily in https://trac.osgeo.org/grass/changeset/10790 then removed again in https://trac.osgeo.org/grass/changeset/10793.

None of these variables should have been exported to the environment in over a decade.

landam commented 5 years ago

Comment by wenzeslaus on 20 May 2015 13:42 UTC Replying to [comment:1 glynn]:

Replying to [ticket:2681 wenzeslaus]:

Especially in older and stable code like grass.py we still keep the legacy LOCATION variable with meaning "full path to a Mapset".

The bug https://trac.osgeo.org/grass/ticket/2679 suggests that nobody is actually using it. So, there is no need for keeping this for backward compatibility anymore.

Originally, GISDBASE, LOCATION_NAME and MAPSET were actual environment variables. [...]

This makes sense. Thanks for the clarification.

...the startup script stopped setting the environment variables (https://trac.osgeo.org/grass/changeset/10655)... The export for LOCATION was re-added temporarily in https://trac.osgeo.org/grass/changeset/10790 then removed again in https://trac.osgeo.org/grass/changeset/10793.

There is some export somewhere even now. I can do:

GRASS > echo $LOCATION
.../grassdata/nc_spm_08_grass7/user1

It seems that [source:grass/trunk/lib/init/grass.py#L1421 bash_startup()] is to blame. It seems that this was added in https://trac.osgeo.org/grass/changeset/60216. I don't see it in prompt.py which was removed in that commit.

None of these variables should have been exported to the environment in over a decade.

And they are not. However, as described in https://trac.osgeo.org/grass/ticket/2679, http://grass.osgeo.org/grass70/manuals/grass7.html#other-examples the documentation says that you can set these environment variables including LOCATION //before// starting GRASS and that GRASS will respect them (i.e. store them into "gisrc" file).

So then I would say that if https://trac.osgeo.org/grass/ticket/2679 functionality will be implemented (rather then dropped) it should be done in the way that

The documentation of grass.py (grass7 --help) can be changed without any harm. (As well as any source code such as grass.py.)

This would leave the "gisrc" file the only occurrence of LOCATION_NAME. As I mentioned before, we cannot remove it because we've already released GRASS GIS 7.0 and the "gisrc" file is used as an interface when "not starting GRASS explicitly." We can do this for GRASS GIS 8.

What we could do for 7 series is to introduce forward compatibility and provide LOCATION as an alternative for LOCATION_NAME. This would require changes to g.gisenv and related library functions but it seems possible (although I haven't checked source code yet).

landam commented 5 years ago

Comment by glynn on 21 May 2015 08:22 UTC Replying to [comment:2 wenzeslaus]:

There is some export somewhere even now. I can do:

GRASS > echo $LOCATION
.../grassdata/nc_spm_08_grass7/user1

It seems that [source:grass/trunk/lib/init/grass.py#L1421 bash_startup()] is to blame.

There is no export. LOCATION is a shell variable, not an environment variable.

So then I would say that if https://trac.osgeo.org/grass/ticket/2679 functionality will be implemented (rather then dropped)

I see no reason to implement it. And if it is implemented, the startup script should explicitly remove those variables from the environment.

This would leave the "gisrc" file the only occurrence of LOCATION_NAME

That's already the case.

What we could do for 7 series is to introduce forward compatibility and provide LOCATION as an alternative for LOCATION_NAME. This would require changes to g.gisenv and related library functions but it seems possible (although I haven't checked source code yet).

Unless you're going to make g.gisenv automatically map LOCATION to LOCATION_NAME, it would require changes to anything which uses it (e.g. lib/python/script and a number of scripts).

landam commented 5 years ago

Comment by wenzeslaus on 9 Jun 2018 21:20 UTC In [changeset:"72791\"] https://trac.osgeo.org/grass/changeset/72791:

init: remove broken env var interface for d/l/m

The interface described in the manual was not implemented and not part of --help.
It seems that it was broken at least since trans from Bash to Python (https://trac.osgeo.org/grass/changeset/37863)
and definitevely before refactoring in https://trac.osgeo.org/grass/changeset/65235. Perhaps some time after env vars
were definitevely removed from modules (https://trac.osgeo.org/grass/changeset/10655, https://trac.osgeo.org/grass/changeset/10790, https://trac.osgeo.org/grass/changeset/10793).

Closes https://trac.osgeo.org/grass/ticket/2679 (Drop or fix setting...) and see #169 for more explanation
esp. about the unfortunate naming (LOCATION vs LOCATION_NAME).

This removes the only code which seemed to be really specific for
the grass - call and it removes a significat portion of the manual
which discusses usage and env vars and command line params priorities 
in various combinations.
landam commented 5 years ago

Comment by wenzeslaus on 10 Jun 2018 01:12 UTC As for using the name LOCATION to mean "full path to a mapset", remaining places are the following:

The main in the variable defined for the shell in lib/init/grass.py. That seems to be only interface-related usage. I think this should be replaced by something else like MAPSET_PATH or FULL_MAPSET.

There is something in lib/init/README which would likely use general update.

There might be some bad examples in existing code, these two may qualify: raster/r.mapcalc/testsuite/const_map_test.sh and testsuite/raster/raster_md5test.sh.

There are places which need deeper look, namely lib/gis/make_mapset.c and lib/gis/location.c.

landam commented 5 years ago

Comment by wenzeslaus on 17 Sep 2018 01:15 UTC In [changeset:"73352\"] https://trac.osgeo.org/grass/changeset/73352:

init: use MAPSET_PATH instead of LOCATION for full path to current mapset variable (see #169)

Changes the (local) shell variable used to refer to full path to the
current mapset from LOCATION to MAPSET_PATH. The name LOCATION was
a lagacy name (see also LOCATION_NAME) reintroduced in https://trac.osgeo.org/grass/changeset/60216
(not mentioned in the commit message, used locally like in the Python prompt).
The LOCATION is kept for backwards compatibility and can be safely removed
for the next major release (8.0).
landam commented 5 years ago

Comment by wenzeslaus on 17 Sep 2018 01:33 UTC In [changeset:"73353\"] https://trac.osgeo.org/grass/changeset/73353:

init: use LOCATION to stand for location name in help (see #169)

In the documentation of CLI and --help use simply LOCATION instead of LOCATION_NAME
because LOCATION fits with mapset. LOCATION_NAME versus LOCATION is a legacy which
does not have to be propagated to primary interface. This changes only documentation,
no iterface changes are made.
landam commented 5 years ago

Comment by wenzeslaus on 17 Sep 2018 02:18 UTC In [changeset:"73356\"] https://trac.osgeo.org/grass/changeset/73356:

init: improve message when d/l/m not set (see #169)

This state is not likely to happen (not sure when it happens),
but giving a general message with general terminology and suggestion
seems like the right thing to do.
landam commented 5 years ago

Comment by wenzeslaus on 17 Sep 2018 02:30 UTC In [changeset:"73357\"] https://trac.osgeo.org/grass/changeset/73357:

tests: don't use LOCATION to mean full path to mapset (see #169)
landam commented 5 years ago

Comment by wenzeslaus on 17 Sep 2018 02:49 UTC Here is the status of the files identified in the comment:5:

Resolved:

Resolved, needs change for 8.0:

Unresolved:

landam commented 5 years ago

Comment by neteler on 30 Nov 2018 11:44 UTC In [changeset:"73729\"] https://trac.osgeo.org/grass/changeset/73729:

init: use LOCATION to stand for location name in help (see #169, backport of trunk https://trac.osgeo.org/grass/changeset/73353)

In the documentation of CLI and --help use simply LOCATION instead of LOCATION_NAME
because LOCATION fits with mapset. LOCATION_NAME versus LOCATION is a legacy which
does not have to be propagated to primary interface. This changes only documentation,
no iterface changes are made.
landam commented 5 years ago

Comment by neteler on 30 Nov 2018 11:50 UTC In [changeset:"73730\"] https://trac.osgeo.org/grass/changeset/73730:

init: improve message when d/l/m not set (see #169, backport trunk https://trac.osgeo.org/grass/changeset/73356)

This state is not likely to happen (not sure when it happens),
but giving a general message with general terminology and suggestion
seems like the right thing to do.
landam commented 5 years ago

Comment by neteler on 30 Nov 2018 11:51 UTC In [changeset:"73731\"] https://trac.osgeo.org/grass/changeset/73731:

tests: don't use LOCATION to mean full path to mapset (see #169)
landam commented 5 years ago

Comment by neteler on 30 Nov 2018 11:57 UTC Selected backports to relbranch76 to avoid that the startup scripts differs too much in the long run as well as getting improvements into the upcoming 7.6.0 release:

Not (yet?) backported: https://trac.osgeo.org/grass/changeset/73352

landam commented 5 years ago

Comment by neteler on 30 Nov 2018 12:45 UTC Also not yet backported: https://trac.osgeo.org/grass/changeset/73354 and https://trac.osgeo.org/grass/changeset/72791