Clear PATH cache when mutating PATH #2508
Conversation
last-genius
force-pushed
the
asv/path-cache
branch
from
43ab16f to
76459da
Compare
andychu
left a comment
andychu
left a comment
There was a problem hiding this comment.
This looks nice and simple! Thank you
One issue is that YSH uses ENV.PATH
$ ysh -c 'echo $[ENV.PATH]; setvar ENV.PATH = "z"; ls'
/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:/home/andy/bin:/wedge/oils-for-unix.org/pkg/python2/2.7.18/bin/
echo $[ENV.PATH]; setvar ENV.PATH = "z"; ls
^~
[ -c flag ]:1: Command 'ls' not found (OILS-ERR-100)
[ -c flag ]:1: errexit PID 2636547: Command failed with status 127
Actually I think a first step would be to add a failing test ... the hook may be a bit trickier (?)
I think spec/path-cache can be folded into spec/command_, where other cases for the PATH cache are
And then maybe add an analogous file spec/ysh-command for the YSH path cache test
last-genius
force-pushed
the
asv/path-cache
branch
from
76459da to
b851265
Compare
|
Added a test for YSH (and made it pass), and in the process discovered there's another way of doing PATH mutation in osh, so added another test and fixed that as well. |
osh/cmd_eval.py
Outdated
| loc.Missing) | ||
| # Hack: let SearchPath know it needs to reset the path | ||
| # cache before looking up a command | ||
| if key == 'PATH': |
There was a problem hiding this comment.
Hm let me think about this ... it seems a bit overzealous, since it also clears the cache on setvar other.PATH = 42, I think
We could define YSH so you do hash -r aka hash --clear or something ... although maybe that's too unfriendly
|
I think it would be OK to land a fix for OSH first, and then have the failing spec test for YSH as a TODO So we're not blocked on a design issue Although I guess this one doesn't necessarily make any regtest/aports pass -- although it might! |
last-genius
force-pushed
the
asv/path-cache
branch
from
b851265 to
5ca553d
Compare
|
Dropped the |
andychu
left a comment
andychu
left a comment
There was a problem hiding this comment.
Hm now I'm thinking about what we need for LANG=C ... hm
I guess for most of these, we tend to CACHE the parsing
e.g. GLOBIGNORE and PATH
We don't do anything special when you mutate them ...
But LANG=C and PATH= are different!
|
|
||
| # Hack: let SearchPath know it needs to reset the path | ||
| # cache before looking up a command | ||
| if lval.name == 'PATH': |
There was a problem hiding this comment.
I think we should leave this out too -- this is in SetNamedYsh, so I think it only happens in YSH
In YSH, setvar PATH =' foo' is just a normal variable
setvar ENV.PATH would be the thing to clear the cache -- we have to think about that a bit
Although it's complicated because you can shopt --set ysh:all and so forth ... gah
There was a problem hiding this comment.
You can do setglobal PATH in osh
There was a problem hiding this comment.
OK good point, that is why I'm thinking about the invariants here
It is a fuzzy line between OSH and YSH
And I even wonder if we need something besides ENV.PATH in YSH ...
| can also be used to initialize? | ||
| """ | ||
|
|
||
| # Hack: let SearchPath know it needs to reset the path |
There was a problem hiding this comment.
I think the assumption here is that this is the ONLY way to change PATH
We also have an issue with LANG=C -- in bash, that doesn't just mutate a variable, it also makes a function call like setlocale() !!
Move the PATH cache test from divergence.test.sh to command_.test.sh Add a new test verifying that changing PATH before builtin exec also invalidates the PATH cache Signed-off-by: Andrii Sultanov <[email protected]>
Mem.SetNamed sets self.clear_path_cache every time 'PATH' is modified. SearchPath checks for self.mem.clear_path_cache on every CachedLookup, clearing the cache if necessary. Fixes: #2445 Signed-off-by: Andrii Sultanov <[email protected]>
Verifies if changing PATH clears the PATH cache. Based on spec/command_.test.sh tests, modified to use setglobal ENV.PATH (removing tests for same-line PATH changes and export since these are not supported in ysh) Both of these tests are currently failing Signed-off-by: Andrii Sultanov <[email protected]>
This is currently failing. Signed-off-by: Andrii Sultanov <[email protected]>
Both SetNamedYsh and SetNamed need to have this hack - the former is invoked in osh with 'setglobal PATH', the latter with 'PATH=' Signed-off-by: Andrii Sultanov <[email protected]>
last-genius
force-pushed
the
asv/path-cache
branch
from
5ca553d to
774a373
Compare
|
Rebased and fixed conflicts |
Fixes: #2445