Make run_script args[0] the absolute path to the script being called#6971
Make run_script args[0] the absolute path to the script being called#6971logan-bobo wants to merge 3 commits intopython-poetry:masterfrom logan-bobo:fix/run-script-absolute-path
Conversation
logan-bobo
changed the title
logan-bobo
added
area/cli
src/poetry/console/commands/run.py
Outdated
| def run_script(self, script: str | dict[str, str], args: str) -> int: | ||
| def run_script(self, script: str | dict[str, str], args: list[str]) -> int: | ||
| env = EnvManager(self.poetry).get() | ||
| args[0] = f"{env.path}/bin/{args[0]}" |
There was a problem hiding this comment.
I am 90% sure that the path will be different for windows. IIRC Windows uses Scripts directory in venvs (please verify it).
There was a problem hiding this comment.
@Secrus thanks for the feedback. I have taken a look and you are right Windows uses the Scripts directory in venvs. I have updated my branch so it will build out the absolute path based on platform. Please see the latest update.
There was a problem hiding this comment.
This looks like we should add a test that fails if the path is wrong. (In general, we should always try to add a test for a bugfix.)
Further, it might be better to use Env.script_dirs than building the path manually. However, I'm not sure which directory to choose if there is more than one.
There was a problem hiding this comment.
I will take a look in to env.script_dirs and also look to add tests to this PR thanks for the feedback 😅
There was a problem hiding this comment.
@radoering it looks as if changing to env.script_dirs[0] works on Linux but not on windows so I am going to leave the paths as {env.path}\Scripts\{args[0]} for Windows and {env.path}/bin/{args[0]} for Linux/MacOS as we know this is where the script will exist defined in tool.poetry.scripts. Will work on extending the MR with test cases now 👐
There was a problem hiding this comment.
What's the issue with script_dirs on Windows?
What I'm actually wondering is if we have to check which folder is the correct one if there are multiple entries in script_dirs or can we be sure that it's script_dirs[0] (or maybe script_dirs[-1])? It might be relevant if env is a VirtualEnv or a SystemEnv.
There was a problem hiding this comment.
According to
poetry/src/poetry/masonry/builders/editable.py
Lines 164 to 172 in ba97fea
script_dirs. Thus, we really should use script_dirs and have to check which is the correct folder.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
4 participants
Pull Request Check List
Resolves: #2435
runcommand functions only deal with edge cases of its use.When a poetry user wants to run tools with auto-reload such as the kind found in Django's
runserverthrough thepoetry runfunctionality the auto-reload will collect the original command again in our case it works aspoetry run scriptwill look up in the config where the script is to be run from however when called again will produce an error as found in #2435. This PR aims to resolve that issue.