Opened 4 years ago

Last modified 12 months ago

#2681 new task

Remove legacy meaning of LOCATION variable

Reported by: wenzeslaus Owned by: grass-dev@…
Priority: blocker Milestone: 8.0.0
Component: Startup Version: svn-trunk
Keywords: init, grass.py, interface, CLI, environment variables Cc:
CPU: Unspecified Platform: All

Description

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 #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 #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.

Change History (15)

comment:1 in reply to:  description ; Changed 4 years ago by glynn

Replying to 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 #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 (r10655), so that unfixed scripts failed rather than using a possibly-incorrect environment. The export for LOCATION was re-added temporarily in r10790 then removed again in r10793.

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

comment:2 in reply to:  1 ; Changed 4 years ago by wenzeslaus

Replying to glynn:

Replying to 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 #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 (r10655)... The export for LOCATION was re-added temporarily in r10790 then removed again in r10793.

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

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

It seems that bash_startup() is to blame. It seems that this was added in r60216. 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 #2679, 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 #2679 functionality will be implemented (rather then dropped) it should be done in the way that

  • LOCATION_NAME is not supported
  • LOCATION means Location name (analogy to MAPSET meaning Mapset name)
  • MAPSET_PATH means full path to mapset (if implemented)
  • variables are removed from the environment by grass.py because no GRASS processes should rely on them
  • we consider prefixing all with GRASS_

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).

comment:3 in reply to:  2 Changed 4 years ago by glynn

Replying to wenzeslaus:

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

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

It seems that 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 #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).

comment:4 Changed 18 months ago by wenzeslaus

In 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 (r37863)
and definitevely before refactoring in r65235. Perhaps some time after env vars
were definitevely removed from modules (r10655, r10790, r10793).

Closes #2679 (Drop or fix setting...) and see #2681 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.

comment:5 Changed 18 months ago by wenzeslaus

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.

comment:6 Changed 14 months ago by wenzeslaus

In 73352:

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

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 r60216
(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).

comment:7 Changed 14 months ago by wenzeslaus

In 73353:

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

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.

comment:8 Changed 14 months ago by wenzeslaus

In 73356:

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

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.

comment:9 Changed 14 months ago by wenzeslaus

In 73357:

tests: don't use LOCATION to mean full path to mapset (see #2681)

comment:10 in reply to:  5 Changed 14 months ago by wenzeslaus

Here is the status of the files identified in the comment:5:

Resolved:

Resolved, needs change for 8.0:

Unresolved:

comment:11 Changed 12 months ago by neteler

In 73729:

init: use LOCATION to stand for location name in help (see #2681, backport of trunk r73353)

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.

comment:12 Changed 12 months ago by neteler

In 73730:

init: improve message when d/l/m not set (see #2681, backport trunk r73356)

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.

comment:13 Changed 12 months ago by neteler

In 73731:

tests: don't use LOCATION to mean full path to mapset (see #2681)

comment:14 Changed 12 months ago by neteler

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: r73352

comment:15 Changed 12 months ago by neteler

Also not yet backported: r73354 and r72791

Note: See TracTickets for help on using tickets.