nat-n / poethepoet

A task runner that works well with poetry.
https://poethepoet.natn.io/
MIT License
1.4k stars 58 forks source link

Included task envfile doesn't consider cwd #160

Closed rjaduthie closed 4 months ago

rjaduthie commented 1 year ago

Summary

Circumstances

When including a task from another file definition - e.g. ./subproject/pyproject.toml - and using the cwd property in the root pyproject.toml.

Effect

If an envfile is used in the task definition in the submodule project, the path to the envfile doesn't consider the cwd when it's imported.

To recreate

Assuming there's an executable my_task and an env file ./subproject/.env.my_task:

./pyproject.toml:

[[tool.poe.include]]
path = "subproject/pyproject.toml"
cwd = "subproject"

./subproject/pyproject.toml

[tool.poe.tasks.my_task]
envfile = ".env.my_task"
cmd = "my_task"

Execution of poetry run poe my_task in the root project will have the variables as defined in ./subproject/.env.my_task.

Possible solutions

I've had a bit of a look around the code. Possibly when adding envs to the EnvVarsManager, the cwd can be respected for include'd tasks?

The code is quite complex (... I haven't yet worked out the interaction with the _get_upstream_invocations etc.), so I haven't got a PR for this issue. I might have so time to look later, but hope that I can get some pointers to how to approach this fix.

rjaduthie commented 1 year ago

(I thought I found that the issue was with a virtualenv, hence closing, but it actually seems not.)

nat-n commented 1 year ago

Hi @rjaduthie, thanks for raising this!

I see how that could be surprising. If I understand correctly what you're demonstrating then I guess the solution would be to make this line do something a bit like this one. You want to have go at it?

One complication with this however is that one might indeed wish for included tasks to be executed in a different directory, but still manage all the local (non checked in) .env files in the project root. So it be nice if we could work out how to have it both ways. Some ideas:

  1. always search both locations with a certain precedence
  2. support templating env vars into the value for envfile, e.g. envfile = "${POE_PROJECT}/.env" or create a new env var to also support envfile = "${POE_INCLUDE}/.env" I suspect the first option makes sense for your use case? but we could do both... What do you think?
roger-duthie-pivotal commented 1 year ago

Thanks for the hints. I'll try to take a look at this at some point and suggest a PR to fix.

In a follow up, I think there's a similar effect with the specification of commands as well. Using the example from my original post, if the subproject task uses a sequence, then the cwd directive is not observed - i.e.

[tool.poe.tasks.my_task]
sequence = 
[
  reference_task,
  { cmd = "scripts_dir_within_subproject/my_task.sh" },
]

... here the second task within the sequence is within a subdir of the subproject, but Poe doesn't "include" it with the correct cwd.

roger-duthie-pivotal commented 1 year ago

To respond to your question about the design options:

I wonder how obvious/intuitive it could be to a user if implementing the first option.

The second option could also get tricky, if you want to override an inherited behaviour.

A third option might be to have a flag within a task or the include block that specifies which behaviour to use - e.g. use_original_envfile=true - or something similarly suitable.

nat-n commented 11 months ago

@rjaduthie FYI I've started looking into this. It's more complex than I realised and I think justifies some refactor of how tasks manage and pass certain types of configuration.

nat-n commented 10 months ago

The issue with sequence task members inheriting cwd should be fixed now with v0.24.2

I'm still working on a fix for the issue included task files which involves a larger refactor.

roger-duthie-pivotal commented 6 months ago

Just to say, we started using the updated package and it now works well for setting the working directory in sequenced tasks. Thank you for that fix!

nat-n commented 4 months ago

The 0.26.0 release includes a minor breaking change that addresses the original issue. The directory specified by the cwd option when including config will now be used at the base path for resolving relative paths for envfiles referenced within the included file.

The docs have been updated to reflect this.