Modify Act-Executor to respect our crazy environment vars. #1326
Conversation
* GERBIL_HOME seems to take superiority over GERBIL_PATH
* GERBIL_LOADPATH seems tts over $GERBIL_PATH/lib/
* Gerbil will load from, in order (?)
- $GERBIL_LOADPATH
- $GERBIL_HOME/lib
? $GERBIL_PATH/lib only sometimes?
Help?
✅ Deploy Preview for elastic-ritchie-8f47f9 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
vyzo
left a comment
vyzo
left a comment
There was a problem hiding this comment.
ok, this gave us useful information; but i think it's the wrong way to go about it.
| (_ (unless (file-exists? env-path) | ||
| (create-directory* env-path))) | ||
| ($env-path (string-append "GERBIL_PATH=" env-path)) | ||
| ($load-path (string-append "GERBIL_LOADPATH=" (path-expand "lib" env-path))) |
There was a problem hiding this comment.
this is unnecessary, we already have GERBIL_PATH; we don't need both to point to the same directory.
| (envvars (filter-out-envvar-prefix "SHELL=" envvars))) | ||
| (append [$env-path | ||
| $path | ||
| $load-path |
| (getenv "SHELL" #f))) | ||
| ($shell (and shell (string-append "SHELL=" shell))) | ||
| (envvars (filter-out-envvar-prefix "GERBIL_PATH=" envvars)) | ||
| (envvars (filter-out-envvar-prefix "GERBIL_LOADPATH=" envvars)) |
There was a problem hiding this comment.
no, this doesn't let the user specify.
|
|
||
| (def (execute-process! exe args env envvars) | ||
| (infof "executing process ~a" [exe args env envvars]) | ||
| (set! envvars [(##os-environ) ... envvars ...]) |
There was a problem hiding this comment.
we should avoid this set!; as you know i don't have any mutation aversion, but here it is is unecessary; ; let is much better.
Also of note, mutation is not free; it results in a box being allocated, so it is better to avoid it unless there are clear benefits (e.g. cleaner code, more efficient algorithms, and so on).
There was a problem hiding this comment.
Also, I think the blanket append is incorrect; it clobbers any variables coming from the user.
The correct way to handle it is so filter things that are not defined in the config envvars.
Also, better to do it below with all the other envvar stuff.
Finally I think it is a mistake to blanket pass all supervisor envvars to subprocesses; there are real securtiy considerations here. We should limit the filter to GERBIL envvars probably.
Help?