[run-task] Support interpolating $TASK_WORKDIR in environment variables
#630
Conversation
ahal
force-pushed
the
ahal/push-xzlwzlwtypxp
branch
from
32c7d8b to
ad1bfef
Compare
ahal
force-pushed
the
ahal/push-xzlwzlwtypxp
branch
from
ad1bfef to
512b52c
Compare
ahal
force-pushed
the
ahal/push-xzlwzlwtypxp
branch
from
512b52c to
3dd4b3f
Compare
bhearsum
left a comment
bhearsum
left a comment
There was a problem hiding this comment.
My current use case is setting PIP_CACHE_DIR to an absolute path under the task's workdir. I could add it to the list of hard coded envs, but it would be better to do something like PIP_CACHE_DIR=$TASK_WORKDIR/.cache/pip and have run-task do the interpolation automatically.
Just to make sure I understand fully: this is necessary when running payloads that don't execute through bash? (We already have places in translations where we use $TASK_WORKDIR directly, but that runs through bash, which I suppose is why that works.
| os.environ[k] = os.path.abspath(os.environ[k]) | ||
| # Interpolate environment variables with defined substitution patterns. For | ||
| # example, the variable `CACHE_DIR={task_workdir}/.cache` will be | ||
| # interpolated with the task's working directory. |
There was a problem hiding this comment.
It would be great to get some minimal mentions of this in the docs.
There was a problem hiding this comment.
I agree, but run-task and the run transforms are entirely undocumented atm. Documenting this should be pretty high priority, but I don't want to scope bloat just to have a place to add this.
| # example, the variable `CACHE_DIR={task_workdir}/.cache` will be | ||
| # interpolated with the task's working directory. | ||
| env_subs = { | ||
| "task_workdir": task_workdir, |
There was a problem hiding this comment.
I assume there's a reason that we can't use the upper-cased $TASK_WORKDIR here? It's a bit weird to have both versions of it. If we can't use the upper cased version, I guess this is the best alternative. (Having a completely differently named variable that is actually the same value would be worse...)
There was a problem hiding this comment.
(see my explanation in the main thread, I think it'll make more sense then)
There was a problem hiding this comment.
I guess I'll expand a little bit.. My initial patch did use $TASK_WORKDIR, but then Bastien pointed out that it was strange that ${TASK_WORKDIR} doesn't work. He suggested using os.path.expandvars as a substitute.
I think this would be a good idea, but I also worry that this could break tasks in subtle ways if it accidentally starts substituting things we didn't intend it to. There's also questions about the order environment variables are substituted and what happens if you have variables that depend on each other in a cycle or with a higher depth.
Ultimately I decided that what we are doing here isn't really environment variable substitution, so it would be cleaner not to disguise it as such. I don't know if it's the right call, but I think this is the safest option. Maybe we can change it again later.
There was a problem hiding this comment.
Sorry, I missed this earlier. Thanks for the explanation - sorry I didn't notice it in the other thread!
No, this is necessary to be able to specify absolute paths in the task definition with Specifically for my use case, I want to be able to set: directly in the task definition. Since |
taskgraph now does this automatically as of taskcluster/taskgraph#630
taskgraph now does this automatically as of taskcluster/taskgraph#630
taskgraph now does this automatically as of taskcluster/taskgraph#630
* chore: upgrade to taskgraph 13.0 This should be fairly boring; the biggest thing it gets us is some under the hood things that will allow us to access better cost tracking. * fix: get rid of unnecessary $TASK_WORKDIR prefix before $VCS_PATH taskgraph now does this automatically as of taskcluster/taskgraph#630 * drop taskcluster/requirements.* These files cause us to reinstall taskgraph at the start of each decision task, which is both unnecessary and potentially changes the version being used, which can be confusing. We do currently use these files as part of a couple of taskgraph tests; I've replaced this usage with installing it through Poetry instead, since we already have taskgraph as a dependency there, and the tests run through Poetry anyways.
3 participants
We currently hard code some environment variables that should be turned into absolute paths. But it would be great if we had the ability to do this for any environment variable.
My current use case is setting
PIP_CACHE_DIRto an absolute path under the task's workdir. I could add it to the list of hard coded envs, but it would be better to do something likePIP_CACHE_DIR=$TASK_WORKDIR/.cache/pipand have run-task do the interpolation automatically.