-
-
Notifications
You must be signed in to change notification settings - Fork 538
Update getcommandpath to use the PATH variable set through setenv=PATH=... #1148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@gaborbernat could you please review? |
Could you please help me with the errors that I am seeing. |
https://toxdev.visualstudio.com/tox/_build/results?buildId=1152 - black needed to reformat your changes. Please run |
The other error seems unrelated to your changes on first look but does not occur on master, so this needs looking into more closely. Please run the tests locally and have a closer look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we can consider this to merge we should discuss what the expected behavior is for edge cases, and then add a few tests to check for validity and defend against regressions.
@gaborbernat @obestwalter how come |
Also when I installed this version of |
On |
EDIT : Fixed all the local errors in |
This PR still lacks tests, without it it's small chance of getting accepted. Please add unit test for each feature improvement one by one, taking care of edge cases too. Furthermore I would prefer to have a one place path mangling, now you do it in two places which makes the code hard to maintain. |
@gaborbernat I understand your view, but the following issue was what I faced when I tried to In the following snippet, the path order to be maintained is Lines 514 to 518 in d6cd8d4
But when I try to pass that variable to _normal_lookup , even envbindir would be passed, which in practice/theory is not correct. Hence I put it this way. Please correct me if I am wrong.
I will add test cases, in a while. Could you please let me know why |
@gaborbernat that was an old build. I have made 2 pushes after that. (force pushes). But it is not updating. Does force push not restart the |
The CI sometimes gets stuck, you can push a new commit to try to retrigger it again, here's the latest https://dev.azure.com/toxdev/tox/_build/results?buildId=1156 |
@Bhargavasomu arguably the order should be setenv + (bindir + what is inherited from shell - in case the user does extends via the setenv - otherwise these latter two probably should not be included). That being said maybe we should not allow users to overwrite PATH, only prepend via setenv. @obestwalter @asottile what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took a look at the cod first, let me check the thing I was supposed to read next 😆
else: | ||
env["PATH"] = os.pathsep.join([bin_dir, os.environ["PATH"]]) | ||
# Remove redundantly duplicated directories in PATH without changing order | ||
env["PATH"] = ":".join(list(dict.fromkeys(env["PATH"].split(":")))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is (for instance) incorrect on windows -- use os.pathsep
instead of ':'
-- also it's unnecessary (and imo a surprising behaviour) to remove duplicates so this modification probably shouldn't happen at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(above is true for .split(":")
as well -- quite surprised this patch passes tests on windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not passing on the windows test cases, and you gave me a solution. Thankyou.
hmm yeah overriding From a test isolation perspective (one of tox's goals) it seems odd that you'd add (for instance from the issue) a directory starting at So I guess I'd like to see a usecase that is more reasonable ? Or maybe convince me what you already have is reasonable? 🤔 (not trying to shoot this down, legitimately curious how this would / should work) |
@asottile, one use case that I came across is as follows. If suppose the user installs an application using tox (via a bash script maybe), then he has to modify the path, so that the installed executable can be invoked from anywhere directly by its name insted of the whole absolute path. This is further needed, because the test cases (in my case), make use of the executable by calling it by its name instead of the whole path. |
@asottile , further I am working on writing a test case, which will make things clear. |
@asottile I have added the new testcase. Please check it out. |
I'll be honest I still don't entirely see the reason for this. What you describe sounds like you want to side step the tox as entry point which I find very dangerous. The more I think about it the surer I am that the only reasonable thing which we could allow is injecting elements into the path before the environment path, but after the tox environments bin folder. |
On the subject of simplifying the path I think it's ok and will help decrease log size and debug. We should definitely keep only the first instance of path occurances. We could also remove not existing paths (I'm not entirely settled on this yet though). That being said this change is orthogonal to this PR so maybe let's keep it separate. |
(not sure how relevant it is to tox -- but) I often use a pattern like this when creating virtualenvs in docker: # set up all env variables in one layer
ENV PATH=/venv/bin:$PATH ... ...
RUN virtualenv /venv && pip install ... I could see a case where removing non-existent paths would break stuff @Bhargavasomu have you considered using something like |
@asottile @gaborbernat I'm really confused from all the above comments. Could you please frame it in a better way as to do what next? |
@asottile was alluding to the fact that's probably more robust to alter the PATH from within the tests (via some session fixture e.g.) rather than inside tox. This way your tests will work also for cases when users are not calling the test from within tox (e.g. they do from within an IDE). |
Thankyou for the support. |
Fixes #1146