Project

General

Profile

Bug #1213

GMXRC make dubious assumptions

Added by Erik Marklund about 7 years ago. Updated about 6 years ago.

Status:
Closed
Priority:
Low
Assignee:
Category:
build system
Target version:
Affected version - extra info:
4.6.*
Affected version:
Difficulty:
uncategorized
Close

Description

I'm using gromacs on Tintin at Uppmax, part of the Swedish national infrastructure for supercomputing. When I source GMXRC I run into an error:

[marklund@tintin3 461]$ source ~/gmx/461_argyris/bin/GMXRC
-bash: goto: command not found

GMXRC, on the other hand states that
"# only csh/tcsh set the variable $shell (note: lower case!)
test $shell && goto CSH",

but indeed my $shell evaluates to bash:
[marklund@tintin3 461]$ echo $shell
bash

It seems that the admins at Uppmax have automatically set the variable for me (I know that I haven't set it myself), but I wouldn't be surprised to see other admins on other HPC centres make similar tweaks. Perhaps GMXRC should revert to the 4.5.6 style, i.e.:

  1. only csh/tcsh understand 'set'
    set is_csh = 123
    test "$is_csh" = 123 && goto CSH

Related issues

Related to GROMACS - Bug #1044: GMXRC alters the contents of the shell's special parametersClosed11/21/2012

Associated revisions

Revision fdeca4a9 (diff)
Added by Teemu Murtola over 6 years ago

More robust shell detection in GMXRC

To cater for environments that set $shell for a non-csh shell, also
check the contents of the variable.

Fixes #1213

Change-Id: I9135b947529b690a733ea0d7be5e8e507236a735

History

#1 Updated by Erik Marklund about 7 years ago

  • Target version changed from 4.6 to 4.6.2

#2 Updated by Mark Abraham about 7 years ago

I have seen this issue mentioned by a user on gmx-users, too.

#3 Updated by Erik Marklund about 7 years ago

I knew I had seen it somewhere before.

So what was the reason to change these few lines in 4.6 anyway? I can revert the change as soon as Jenkins is back online unless there were problems with that solution.

#4 Updated by Mark Abraham about 7 years ago

Erik Marklund wrote:

I knew I had seen it somewhere before.

So what was the reason to change these few lines in 4.6 anyway? I can revert the change as soon as Jenkins is back online unless there were problems with that solution.

git blame is one's friend here. Turns out your #1044 provoked this change :-)

#5 Updated by Erik Marklund about 7 years ago

Haha! Wonderful. Ok, let's not revert it. I don't even understand how that commit could fix #1044 in the first place.

How about conditionally checking the contents of $shell? If $shell is set and the shell is csh/tcsh then $shell should expand to one of those, non?

#6 Updated by Mark Abraham about 7 years ago

It seems like we should be a little more sophisticated, yes. http://stackoverflow.com/questions/3327013/how-to-determine-the-current-shell-im-working-on seems to suggest there is no perfect solution. And whatever test we do use should work in a shell-independent way on (at least) bash, t?csh, and ksh, as well as sh that is actually any of those. Ugh. :-)

#7 Updated by Teemu Murtola almost 7 years ago

Erik Marklund wrote:

How about conditionally checking the contents of $shell? If $shell is set and the shell is csh/tcsh then $shell should expand to one of those, non?

An intermediate version of the change in gerrit actually had code to check the contents of $shell, so that could be a reasonable way forward.

#8 Updated by Mark Abraham almost 7 years ago

  • Target version changed from 4.6.2 to future
  • Affected version set to 4.6.1

#9 Updated by Mark Abraham almost 7 years ago

  • Target version changed from future to 4.6.x

#10 Updated by Christoph Junghans almost 7 years ago

Also see comments in:
<https://gerrit.gromacs.org/#q,I852d075c80e2bf8f81a02932432dd31b3bab7f09,n,z>

echo $shell | grep csh >/dev/null && goto CSH

might be a slightly more safe solution.

#11 Updated by Pedro Lacerda almost 7 years ago

Christoph Junghans wrote:

echo $shell | grep csh >/dev/null && goto CSH

might be a slightly more safe solution.

Or just:

echo $shell | grep -q csh && goto CSH

#12 Updated by Teemu Murtola over 6 years ago

  • Status changed from New to Fix uploaded
  • Assignee set to Teemu Murtola
  • Target version changed from 4.6.x to 5.0
  • Affected version - extra info changed from 4.6.1 to 4.6.*

Will upload the fix proposed in comments 7/10/11 shortly.

#13 Updated by Gerrit Code Review Bot over 6 years ago

Gerrit received a related patchset '1' for Issue #1213.
Uploader: Teemu Murtola ()
Change-Id: I9135b947529b690a733ea0d7be5e8e507236a735
Gerrit URL: https://gerrit.gromacs.org/2947

#14 Updated by Teemu Murtola over 6 years ago

  • Status changed from Fix uploaded to Resolved

#15 Updated by Teemu Murtola about 6 years ago

  • % Done changed from 0 to 100

#16 Updated by Rossen Apostolov about 6 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF