Remove erroneous environ commands #1584
Conversation
| environ = os.environ.copy().update( | ||
| HOME=env.paths['home'], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Don't you need to remove the references in calls to environment.run_setup_py(..., env=environ) too?
`environ.update()` mutates a list in place and returns `None`, so all of these blocks with `os.environ.copy().update(...)` are creating an ephemeral copy of the environment dictionary, updating it, and then it's deleted since nothing holds a reference to it. Since these were no-ops anyway, I've removed them.
pganssle
force-pushed
the
environ_update
branch
from
de4ea91 to
2e8e529
Compare
|
Hm, I guess there's It's a bit confusing how the Also, presumably this was intended to do a specific thing. Maybe we should be setting |
|
Actually, I think I can actually just easily make this work as intended, though it's disturbing that this has been broken for 4 years (and that everyone's just been cargo-culting this the whole time - almost all the instances of this are from blind copy-pasting). Not sure how to test that the fixed version actually works, though. Probably seeing if some configuration in my home directory doesn't get picked up after it's fixed? |
|
Interestingly, the tests actually fail if @jaraco Looks like this commit causes the problems because it changes everything over to assuming that the I think I can fix it by looking for all the |
I did not have any particular expectations about the tests. I do agree that it's perhaps surprising and undesirable to be generating files in the cwd unless that directory was explicitly set to some temporary directory for the duration of the test. Since the I don't understand the meaning/purpose of HOME. I would expect HOME only to affect the reading of
Perhaps that's the right thing to do, but it feels like a workaround for earlier bad behavior. I'm not seeing the problem clearly, though. Feel free to do whatever you think is right. |
This is indeed already happening. The
I also don't understand why this is happening, I assumed it was deliberate because you created an
I am just trying to do some software archaeology. I was surprised by the current behavior and wanted to check with you to see if you had any better information about you removed |
|
I don't have any better information, sorry (and sorry for the delayed response). |
In #1559, @RajdeepRao pointed out that these code blocks were in many tests here.
environ.update()mutates a list in place and returnsNone, so all of these blocks withos.environ.copy().update(...)are creating an ephemeral copy of the environment dictionary, updating it, and then it's deleted since nothing holds a reference to it.Since these were no-ops anyway, I've removed them. I believe the
envfixture defined in this file should be used instead.Pull Request Checklist