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: Evaluate ^export lines only and expand variables in double/non-quoted values #170

Merged
merged 7 commits into from
Dec 4, 2019

Conversation

HuidaeCho
Copy link
Member

This PR properly evaluates ^export in single lines in ~/.grass7/bashrc. It will ignore multi-line environment variables. Without this PR,

alias vi='vim'
export GDAL_DRIVER_PATH="$HOME/gdalplugins"
#export IGNORE=ME
export HOHO='$HOME'

defines

vi='vim' # it's not meant to be an environment variable
GDAL_DRIVER_PATH="$HOME/gdalplugins" # not /home/user/gdalplugins
                                     # note double quotes in the definition itself
IGNORE=ME # shouldn't be defined
HOHO='$HOME' # not just $HOME

With this PR, the same lines would define

GDAL_DRIVER_PATH=/home/user/gdalplugins # $HOME in double quotes expanded
HOHO=$HOME # $HOME in single quotes not expanded

@HuidaeCho
Copy link
Member Author

HuidaeCho commented Oct 30, 2019

Non ^export variables will be written into .bashrc in the mapset directory and get evaluated by bash. For example,

case $TERM in
xterm*|screen)
        export PS1="\[\033]0;\$(grass_ps) \w\007\]\$(grass_ps) \w> "
        ;;
esac

This PS1 doesn't start with export (tab first) and won't be evaluated by load_env(). Instead, bash_startup() will write it into .bashrc.

@HuidaeCho HuidaeCho added bug Something isn't working enhancement New feature or request labels Oct 31, 2019
@HuidaeCho HuidaeCho self-assigned this Oct 31, 2019
@neteler
Copy link
Member

neteler commented Nov 3, 2019

Maybe off-topic here but I'd love to see that the standard aliases defined in ~/.bashrc would be visible within a GRASS GIS session...

@HuidaeCho
Copy link
Member Author

Maybe, we can source ~/.bashrc, but I'm wondering why we create a new bashrc for each mapset.

@HuidaeCho
Copy link
Member Author

    # instead of changing $HOME, start bash with:
    #   --rcfile "$LOCATION/.bashrc" ?
    #   if so, must care be taken to explicitly call .grass.bashrc et al
    #   for non-interactive bash batch jobs?

Do we change $HOME and start a new bash process for 1. storing history and 2. non-interactive batch jobs from non-regular users (e.g., apache)?

@HuidaeCho
Copy link
Member Author

source ~/.bashrc doesn't work because ~ is $LOCATION.

HOME=/home/user
source ~/.bashrc

works, but if the whole purpose of this new bash process is for non-interactive jobs... I'm not sure if you want to do this.

@HuidaeCho
Copy link
Member Author

I moved export HOME= above .grass.bashrc so you can source ~/.bashrc.
But I'm still wondering why we separate out export parsing (load_env) from the rest. Any reason for setting environment variables first and then loading the rest of .grass.bashrc?

@neteler
Copy link
Member

neteler commented Nov 15, 2019

Maybe, we can source ~/.bashrc, but I'm wondering why we create a new bashrc for each mapset.

I have not clue... maybe a relict from ancient times?

@HuidaeCho
Copy link
Member Author

Maybe, we can source ~/.bashrc, but I'm wondering why we create a new bashrc for each mapset.

I have not clue... maybe a relict from ancient times?

Maybe for history in each mapset?

@HuidaeCho
Copy link
Member Author

This PR should allow sourcing ~/.bashrc from ~/.grass7/bashrc and fix the issues mentioned in the first comment.

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.

Please, add comments, so it is easier to investigate the code later.

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

wenzeslaus commented Nov 26, 2019

I agree with this PR that the current code needs revising. If you are a command line user, you expect to get your aliases. The home switch code just seems hacky. We were discussing this in the past both the home and the aliases. Here are some links and quotes (mixed from all threads):

[GRASS-dev] [GRASS-SVN] r61051 - grass/trunk/lib/init
https://lists.osgeo.org/pipermail/grass-dev/2014-July/069986.html

[GRASS-dev] Bash aliases in GRASS
http://lists.osgeo.org/pipermail/grass-dev/2013-February/061837.html

[GRASS-user] loss of bash aliases
http://lists.osgeo.org/pipermail/grass-user/2008-March/043806.html

Vaclav Petras:

what is the reason for not-loading bash aliases and the user bashrc
file when starting new bash? It is meant to prevent conflicts?

Glynn Clements:

AFAICT, the reason for messing about with the shell startup was to
force the use of per-mapset history files.

The code which creates the GRASS-specific .bashrc file tries to load
the aliases, but it does so before HOME has been restored, so it
doesn't work.

If you normally use GRASS from the command line, you're probably
better off just forgetting about the whole "startup" thing and putting
the necessary environment settings into your ~/.bashrc.

Ivan Shmakov:

It may worth to include ~/.bashrc from ~/.grass.bashrc, ... (Just as ~/.bashrc typically sources /etc/bashrc.)

This way, you should get all the aliases, Shell variables & functions defined in ~/.bashrc defined in GRASS as well. (But you'll need to be sure that your ~/.bashrc won't interfere with GRASS.)

Huidae Cho:

... [see the actual thread for a lot of good points]
IMHO, it should be the user's responsibility and rights to overwrite or not overwrite anything available outside a GRASS session and that should happen in ~/.grass7/bashrc.

Vaclav Petras:

I'm not sure if this is related but note that bash aliases and perhaps some other things are not available in GRASS session and it is not considered as an issue.

@HuidaeCho
Copy link
Member Author

HuidaeCho commented Nov 26, 2019

Added comments.

Actually, Glynn made a point. We could just add GRASS-specific environment variables in ~/.bash_profile and do nothing to start GRASS, but I'm not sure if that's a good idea... We want at least some kind of data protection from regular bash uses.

@wenzeslaus
Copy link
Member

Actually, Glynn made a point. We could just add GRASS-specific environment variables in ~/.bash_profile and do nothing to start GRASS, but I'm not sure if that's a good idea... We want at least some kind of data protection from regular bash uses.

I don't think that's a good idea. If we want GRASS GIS modules to be available by default in command line without starting a session, then they should be just normal command line tools installed to the system. However, I don't think that's how it should be done and I would rather do the subcommand interface like version control systems or new ImageMagick.

@HuidaeCho
Copy link
Member Author

Actually, Glynn made a point. We could just add GRASS-specific environment variables in ~/.bash_profile and do nothing to start GRASS, but I'm not sure if that's a good idea... We want at least some kind of data protection from regular bash uses.

I don't think that's a good idea. If we want GRASS GIS modules to be available by default in command line without starting a session, then they should be just normal command line tools installed to the system. However, I don't think that's how it should be done and I would rather do the subcommand interface like version control systems or new ImageMagick.

Totally agreed. Since this PR only fixes unintended bugs, I'll merge it for now if it works on your machine.

@HuidaeCho
Copy link
Member Author

I've addressed Vaclav's comments and, since there are no more comments, I'll take the liberty and merge this PR.

@HuidaeCho HuidaeCho merged commit 7316936 into OSGeo:master Dec 4, 2019
@neteler
Copy link
Member

neteler commented Dec 13, 2019

Backported to relbranch78

@HuidaeCho HuidaeCho deleted the load_env branch December 15, 2019 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants