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

What is the rationale behind quietly ignoring Environment.SetEnvironmentVariable calls with targets User or Machine on Unix-like platforms? #32685

Closed
mklement0 opened this Issue Oct 8, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@mklement0

mklement0 commented Oct 8, 2018

Persistently updating environment variables is (at least currently) only supported on Windows, which means passing the User and Machine enumeration values to Environment.SetEnvironmentVariable are unsupported on macOS and Linux.

Surprisingly, however, doing the latter doesn't cause an exception, but is quietly ignored (an effective no-op).

Even though this behavior is documented, I wonder:

  • what the rationale for this behavior is, given that as an API user you generally want to be notified of attempts to use functionality that is unsupported on a given platform.

  • whether the behavior can be changed.

@mklement0 mklement0 changed the title from What is the rationale behind quietly ignoring Environment.SetEnvironmentVariable with targets User and Machine on Unix-like platforms? to What is the rationale behind quietly ignoring Environment.SetEnvironmentVariable calls with targets User or Machine on Unix-like platforms? Oct 8, 2018

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Oct 8, 2018

Member

See the discussion in #20147

Member

jkotas commented Oct 8, 2018

See the discussion in #20147

@mklement0

This comment has been minimized.

Show comment
Hide comment
@mklement0

mklement0 Oct 8, 2018

Thanks, @jkotas.

Reading the discussion and the linked issues I gather that the plan for addressing this is the following, correct?:

mklement0 commented Oct 8, 2018

Thanks, @jkotas.

Reading the discussion and the linked issues I gather that the plan for addressing this is the following, correct?:

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Oct 8, 2018

Member

Right, that is the current plan.

Member

jkotas commented Oct 8, 2018

Right, that is the current plan.

@mklement0

This comment has been minimized.

Show comment
Hide comment
@mklement0

mklement0 Oct 8, 2018

I see, thanks.

Is the reason for not throwing a PNSE at runtime compatibility with Mono?

Note that not all CoreFx consumers are compiled languages, so calls from PowerShell code, for instance, would not see the compile-time warning. A cmdlet that wraps this API, such as proposed in PowerShell/PowerShell#5340 (comment), is then faced with the following - suboptimal - choices:

  • Accept the quiet no-op runtime behavior on unsupported platforms.

  • Duplicate the compile-time warnings by turning them into PowerShell errors / warnings.

mklement0 commented Oct 8, 2018

I see, thanks.

Is the reason for not throwing a PNSE at runtime compatibility with Mono?

Note that not all CoreFx consumers are compiled languages, so calls from PowerShell code, for instance, would not see the compile-time warning. A cmdlet that wraps this API, such as proposed in PowerShell/PowerShell#5340 (comment), is then faced with the following - suboptimal - choices:

  • Accept the quiet no-op runtime behavior on unsupported platforms.

  • Duplicate the compile-time warnings by turning them into PowerShell errors / warnings.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Oct 8, 2018

Member

Is the reason for not throwing a PNSE at runtime compatibility with Mono?

It is one of the reasons. Even without Mono in the equation, no-op seemed like the right choice for this one because of it makes more code "just work" (#20147 (comment)). There are other methods like this in the framework that we choose to turn into no-op on non-Windows platforms. The choices for these Windows-specific methods are always sub-optimal. Ideally, they would be in Windows-specific assembly.

Changing the behavior to throw PNSE would be a breaking change.

Member

jkotas commented Oct 8, 2018

Is the reason for not throwing a PNSE at runtime compatibility with Mono?

It is one of the reasons. Even without Mono in the equation, no-op seemed like the right choice for this one because of it makes more code "just work" (#20147 (comment)). There are other methods like this in the framework that we choose to turn into no-op on non-Windows platforms. The choices for these Windows-specific methods are always sub-optimal. Ideally, they would be in Windows-specific assembly.

Changing the behavior to throw PNSE would be a breaking change.

@mklement0

This comment has been minimized.

Show comment
Hide comment
@mklement0

mklement0 Oct 8, 2018

Thanks, @jkotas.

Understood re breaking change, so I'm closing this.

Sounds like moving such overloads / methods into Windows-specific assemblies is the way to go in the future.

In cases where this is impractical, please reconsider the current "just works" approach, because its proper name should be "doesn't work, but pretends that it does", which I think is more harmful than throwing an exception.
(A compiler warning, which is helpful irrespective of the runtime behavior, can mitigate this, but those warnings aren't always seen, e.g., in PowerShell).

mklement0 commented Oct 8, 2018

Thanks, @jkotas.

Understood re breaking change, so I'm closing this.

Sounds like moving such overloads / methods into Windows-specific assemblies is the way to go in the future.

In cases where this is impractical, please reconsider the current "just works" approach, because its proper name should be "doesn't work, but pretends that it does", which I think is more harmful than throwing an exception.
(A compiler warning, which is helpful irrespective of the runtime behavior, can mitigate this, but those warnings aren't always seen, e.g., in PowerShell).

@mklement0 mklement0 closed this Oct 8, 2018

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Oct 9, 2018

Member

Arguably it could at least update the process environment, since in some cases this would happen on Windows (depends on how many apps respect the WM_SETTINGCHANGE message)

Member

danmosemsft commented Oct 9, 2018

Arguably it could at least update the process environment, since in some cases this would happen on Windows (depends on how many apps respect the WM_SETTINGCHANGE message)

@mklement0

This comment has been minimized.

Show comment
Hide comment
@mklement0

mklement0 Oct 9, 2018

@danmosemsft:

But the method call itself currently doesn't update the process-level variable directly, so wouldn't what you're proposing be a breaking change?

On a side note: It has always struck me as inconvenient that you need a second call just to make the value effective in the current process as well.

Perhaps it's worth consider extending the enumeration with UserAndProcess and MachineAndProcess values that set both the persisted and the current-process value.

mklement0 commented Oct 9, 2018

@danmosemsft:

But the method call itself currently doesn't update the process-level variable directly, so wouldn't what you're proposing be a breaking change?

On a side note: It has always struck me as inconvenient that you need a second call just to make the value effective in the current process as well.

Perhaps it's worth consider extending the enumeration with UserAndProcess and MachineAndProcess values that set both the persisted and the current-process value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment