[GHC] #12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: | Version: 8.0.1 libraries/base | Keywords: | Operating System: Unknown/Multiple Architecture: | Type of failure: None/Unknown Unknown/Multiple | Test Case: | Blocked By: Blocking: | Related Tickets: Differential Rev(s): | Wiki Page: -------------------------------------+------------------------------------- Relevant code: {{{ -- | @setEnv name value@ sets the specified environment variable to @value@. -- -- On Windows setting an environment variable to the /empty string/ removes -- that environment variable from the environment. For the sake of -- compatibility we adopt that behavior. In particular -- -- @ -- setEnv name \"\" -- @ -- -- has the same effect as -- -- @ -- `unsetEnv` name -- @ }}} This sounds nice and authoritative... and it's also not true. https://msdn.microsoft.com/en- us/library/windows/desktop/ms686206(v=vs.85).aspx states that only if the pointer is NULL (as opposed to an empty string) is the environment variable deleted. https://github.com/golang/go/issues/5610 corroborates So `setEnv` has been given bogus semantics that don't make sense because empty environment variables ARE supported on Windows. My suggestion is to just fix this so that setEnv takes empty environment variables. But this would be a BC breaking change. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by bergmark): I think it's important to make sure that there is some kind of mention/warning to let users know that their code changed because of this. I see three ways to accomplish this: * Deprecate `setEnv` and add the new function under a new name * Add a warning pragma for one release cycle and then make the change * Change the parameter to only accept non-empty strings What about staying compatible with several versions of base? Should users never use an empty string (effectively banning the use of empty env vars), and/or should we refer users to the base-compat package? -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:1> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by hvr): It's maybe worth pointing out that `setEnv` has been introduced with base-4.7.0.0 (via #7427 & https://mail.haskell.org/pipermail/libraries/2012-October/018560.html) Replying to [comment:1 bergmark]:
I think it's important to make sure that there is some kind of mention/warning to let users know that their code changed because of this.
On the one hand, we use semantic versioning to signal this very kind of semantic API changes; on the other hand, `base` makes inflationary use of major version increments due to its large API surface, so that we end up with a sub-optimal signal/noise ratio, resulting in API consumers not paying attention anymore... :-(
I see three ways to accomplish this:
* Deprecate `setEnv` and add the new function under a new name
I think that'd be the safest approach given the circumstances, and it would side-step all the BC issues; the only downside is having to figure out a good name for the new function... :-)
* Add a warning pragma for one release cycle and then make the change
That's still gonna result in silent failures for users which don't pay attention to semantic version signalling nor compile warnings or which happen to skip that one release cycle which warned about it.
* Change the parameter to only accept non-empty strings
how?
What about staying compatible with several versions of base? Should users never use an empty string (effectively banning the use of empty env vars), and/or should we refer users to the base-compat package?
In hindsight, maybe we should have thrown an exception for situations where `setEnv` would act like `unsetEnv`, or unify `setEnv` with `unsetEnv` by using `:: String -> Maybe String -> IO ()`... -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:2> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by bergmark):
On the one hand, we use semantic versioning to signal this very kind of semantic API changes; on the other hand, base makes inflationary use of major version increments due to its large API surface, so that we end up with a sub-optimal signal/noise ratio, resulting in API consumers not paying attention anymore... :-( Do people actually care less about the base changelog than for other packages? Semantic changes are always scary since they can be missed, but your point still stands.
Change the parameter to only accept non-empty strings how? Using NonEmpty Char, which is a bit wonky. ''dreams of non-empty literals''
-- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:3> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by carter): I think it's OK to just change the semantics of this function and make the change clearly marked in the change log and its documentation. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:4> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by ekmett): I also support just fixing this. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:5> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by hvr): Btw, if we go for the first option, i.e. just change semantics of the existing `setEnv` operation (in which case I'd argue not to use any warnings, as we don't have any facility to properly acknowledge specific warnings yet), then we need to make sure that the hackage:setenv package gets retrofitted with an upper bound on `base`, since `setenv` re-exports `setEnv` from `base`... -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:6> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by adamgundry): I'm inclined to agree that we could just change this, and mention in the new docs that the function had different behaviour in older versions of `base`. While we're at it, perhaps it is worth explicitly documenting the fact that `setEnv` is not thread safe? This may be obvious to others, but it wasn't to me, and has been causing segfaults in a production Haskell application. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:7> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by habibalamin): Here's how Ruby handles empty environment variables on Windows — https://github.com/ruby/ruby/commit/2982c5289210c02120172bf631270858681d031d #diff-1f049b4f9a6a39cee9cf18b2dc85f637R3147. Are we agreed on the course of action? -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:8> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: | libraries/base/tests/T12494.hs Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by habibalamin): * testcase: => libraries/base/tests/T12494.hs -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:9> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: | libraries/base/tests/T12494.hs Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3726 Wiki Page: | -------------------------------------+------------------------------------- Changes (by habibalamin): * differential: => Phab:D3726 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:10> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: | libraries/base/tests/T12494.hs Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3726 Wiki Page: | -------------------------------------+------------------------------------- Comment (by Tamar Christina <tamar@…>): In [changeset:"49e334c8ea98cd5ecc81cfe10827538182815723/ghc" 49e334c8/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="49e334c8ea98cd5ecc81cfe10827538182815723" Allow Windows to set blank environment variables Test Plan: ./validate on harbormaster Reviewers: austin, hvr, bgamari, erikd, Phyx Reviewed By: Phyx Subscribers: Phyx, rwbarton, thomie GHC Trac Issues: #12494 Differential Revision: https://phabricator.haskell.org/D3726 }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:11> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: (none) Type: bug | Status: closed Priority: normal | Milestone: Component: libraries/base | Version: 8.0.1 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: | libraries/base/tests/T12494.hs Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3726 Wiki Page: | -------------------------------------+------------------------------------- Changes (by Phyx-): * status: new => closed * resolution: => fixed -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:12> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#12494: Implementation of setenv in base incorrectly claims empty environment variable not supported on Windows -------------------------------------+------------------------------------- Reporter: ezyang | Owner: (none) Type: bug | Status: closed Priority: normal | Milestone: 8.4.1 Component: libraries/base | Version: 8.0.1 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: | libraries/base/tests/T12494.hs Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3726 Wiki Page: | -------------------------------------+------------------------------------- Changes (by Phyx-): * milestone: => 8.4.1 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/12494#comment:13> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
participants (1)
-
GHC