Executors should only setup PATH again in activated state. #2711
Conversation
mitchell-as
force-pushed
the
mitchell/dx-2068-2
branch
from
f94bf10
to
4863bdb
Compare
The other environment variables have already been set.
mitchell-as
force-pushed
the
mitchell/dx-2068-2
branch
from
4863bdb
to
acdee8a
Compare
There was a problem hiding this comment.
Not actually requesting changes. I think we should close this PR and revert the work done here so far, because as you can see from my other comment this still has significant issues. I think we need to take this back to the drawing board first as it feels like we're playing whack a mole without a strategy (that's not on you - that's just the reality of our initial plan falling apart).
| projectCacheDir := filepath.Join(storage.CachePath(), hash.ShortHash(projectDir)) | ||
| if projectCacheDir == execDir { | ||
| transformEnv = false // runtime environment variables already set | ||
| if projectCacheDir == filepath.Dir(execDir) { |
There was a problem hiding this comment.
I realize this wasn't introduced in this PR, but it just occurred to me that this isn't going to work if someone checked out with a custom path.
Additionally, we can't assume the two sides of the path were constructed the same way. We get around this in other places with this function:
cli/internal/fileutils/fileutils.go
Line 1165 in d6bc461
The other environment variables have already been set.
The first attempt at fixing this assumed that
PATHmodifications bystate shellwere complete. However, they were not. Executors also modifyPATHbased on meta information, so that absolutely needs to be done every time an executor is run. Any other environment variables are already set properly, so they can be ignored.