Skip to content
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

Add os.reload_environ() function #120057

Closed
vstinner opened this issue Jun 4, 2024 · 26 comments
Closed

Add os.reload_environ() function #120057

vstinner opened this issue Jun 4, 2024 · 26 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

vstinner commented Jun 4, 2024

Feature or enhancement

When the environment is modified outside Python, os.environ is not updated. I propose adding a new os.environ.refresh() method to manually update os.environ.

Discussion: https://discuss.python.org/t/method-to-refresh-os-environ/54774

Linked PRs

@vstinner vstinner added the type-feature A feature request or enhancement label Jun 4, 2024
vstinner added a commit to vstinner/cpython that referenced this issue Jun 4, 2024
vstinner added a commit to vstinner/cpython that referenced this issue Jun 4, 2024
@eryksun
Copy link
Contributor

eryksun commented Jun 4, 2024

Note that on Windows the C runtime's Unicode and multibyte environments -- _wenviron and _environ -- can get out of sync with the process environment -- GetEnvironmentStringsW() -- due to code that directly calls WinAPI SetEnvironmentVariableW(). It's possible to implement a function to fresh the C environment on Windows, which unsets the variables in _wenviron and sets the variables from GetEnvironmentStringsW(), but that's probably not something that the Python runtime needs to worry about.

vstinner added a commit to vstinner/cpython that referenced this issue Jun 5, 2024
@vstinner
Copy link
Member Author

vstinner commented Jun 5, 2024

but that's probably not something that the Python runtime needs to worry about

As soon as there are different "caches", it's hard to keep everything in sync and consistent :-(

@erlend-aasland
Copy link
Contributor

I'm not sure if this is a good idea. I suspect it will be a hard feature to get right, and as noted in the forum discussion, most beginners will misunderstand the feature.

@vstinner
Copy link
Member Author

vstinner commented Jun 5, 2024

I suspect it will be a hard feature to get right

Would you mind to elaborate?

@vstinner
Copy link
Member Author

vstinner commented Jun 5, 2024

and as noted in the forum discussion, most beginners will misunderstand the feature.

I added "in the same process" to reduce misunderstanding.

@picnixz
Copy link
Contributor

picnixz commented Jun 5, 2024

FYI, there was this issue: #119737 (so you may want to close it to avoid duplicates)

@vstinner
Copy link
Member Author

vstinner commented Jun 5, 2024

I closed issue #119737 as a duplicate of this issue.

@vstinner
Copy link
Member Author

Implemented by the commit 7aff2de.

@WolfDWyc
Copy link

@vstinner

As I noted in the discourse thread, and was mentioned in the comment on the PR, I still feel like this name will be confusing for new users, even with docs clarifying it. Can we maybe re-discuss this? Or at least choose another name?

Should we continue here or in the discourse thread?

@WolfDWyc
Copy link

I've elaborated on this concern here: https://discuss.python.org/t/method-to-refresh-os-environ/54774/24

vstinner added a commit to vstinner/cpython that referenced this issue Jun 14, 2024
vstinner added a commit to vstinner/cpython that referenced this issue Jun 14, 2024
vstinner added a commit to vstinner/cpython that referenced this issue Jun 20, 2024
…)"

This reverts commit 7aff2de.

The function is controversial and it was decided to remove it.
vstinner added a commit to vstinner/cpython that referenced this issue Jun 20, 2024
mrahtz pushed a commit to mrahtz/cpython that referenced this issue Jun 30, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
@picnixz
Copy link
Contributor

picnixz commented Sep 1, 2024

Since we had a bunch of closed PRs, should this issue be closed for now? or are there other pending discussions?

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Sep 2, 2024

There was other ongoing discussion: https://discuss.python.org/t/bikeshedding-a-method-to-refresh-os-environ/57628/22

I think that @ncoghlan's idea about a module level os.reload_environment() function addresses most of concerns about os.environ.refresh(). But the discussion did not stop here, so not all see it so.

Somebody need to put a period here. Either reimplement this feature as a module level function, or keep all as is, -- these are two most probable options.

@vstinner
Copy link
Member Author

Python 3.14 alpha 1 was released with os.environ.refresh() method. What's the status of this issue?

@erlend-aasland
Copy link
Contributor

Python 3.14 alpha 1 was released with os.environ.refresh() method. What's the status of this issue?

You tell me; you created the issue :)

@vstinner
Copy link
Member Author

I closed the issue, I'm not the one who reopened it.

@vstinner
Copy link
Member Author

@serhiy-storchaka: What's the status of this issue?

@serhiy-storchaka
Copy link
Member

You should ask @zooba, the author of the discussion. According to the results of the poll, the majority prefers a module level function. Voting is not binding, and not only core developers voted, but this should still be taken into account. I do not see a vote of @zooba. Is he satisfied with proposed variants and what does he propose?

@vstinner
Copy link
Member Author

@zooba: What's the status of this issue?

@vstinner
Copy link
Member Author

I would suggest to close the issue and keep the os.environ.refresh() method.

@WolfDWyc
Copy link

I think if reload_environment is preferred by more people we should go with that.

@zooba
Copy link
Member

zooba commented Oct 30, 2024

I posted on the discussion, but the majority liked the module-level function instead of putting it on the object, and I personally think reload_environ() is slightly more accurate than reload_environment(), since it is environ that is being reloaded. (There was no poll option for other module-level functions, so I'm assuming people liked os.reload... and disliked os.environ.....)

So as much as anyone wants to keep me happy, os.reload_environ() is probably the best option. Documentation will have to deal with people who read the name and misunderstand what it's for, but that is basically documentation's job.

@serhiy-storchaka
Copy link
Member

On other hand, it reloads the process environment to the os.environ and os.environb mappings.

@ncoghlan
Copy link
Contributor

ncoghlan commented Oct 31, 2024

I think @zooba's point was that this function doesn't reload the system wide environment variable settings into the running application process, which os.reload_environment() could be taken to mean.

That misreading doesn't seem particularly likely to me though, and the fact this operation reloads both os.environ and os.environb (not just os.environ) makes me wary of the shorter os.reload_environ() name (it feels misleading to me in the same way that the os.environ.refresh() spelling is misleading).

@zooba
Copy link
Member

zooba commented Oct 31, 2024

I can't reasonably see os.reload_environ() implying that os.environb wouldn't also be reloaded. Certainly not as much as I can see someone thinking that os.reload_environment() implies something other than "os.environ will be reloaded".

Whereas os.environ.refresh() could quite reasonably be assumed to have no impact on os.environb. So I think that's another argument in favour of the module-level function.

Perhaps os.reload_environs() feels more comfortable for covering both os.environ and os.environb? I don't have a strong preference about the s. My main concern is still the same as at the start - this function doesn't impact the (generic) environment of the process at all, it only affects what is contained in Python's cache of the environment, otherwise known as os.environ.

vstinner added a commit to vstinner/cpython that referenced this issue Nov 1, 2024
Replace the os.environ.refresh() method with a new
os.reload_environ() function.
@vstinner
Copy link
Member Author

vstinner commented Nov 1, 2024

I created PR gh-126268 to replace the os.environ.refresh() method with a new os.reload_environ() function.

IMO reload_environ() is a good name since it reminds os.environ. I don't think that we should bother with the fact that os.environb is also reloaded to name the function. And reload_environment() is too long :-)

@picnixz picnixz added the stdlib Python modules in the Lib dir label Nov 1, 2024
vstinner added a commit that referenced this issue Nov 5, 2024
Replace the os.environ.refresh() method with a new
os.reload_environ() function.

Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
@vstinner vstinner changed the title Add os.environ.refresh() method Add os.reload_environ() function Nov 5, 2024
@vstinner
Copy link
Member Author

vstinner commented Nov 5, 2024

I created PR #126268 to replace the os.environ.refresh() method with a new os.reload_environ() function.

Done: I merged the PR. I close the issue.

@vstinner vstinner closed this as completed Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

8 participants