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

Make behavior of environment.opIndexAssign on Posix to match the behavior on Windows #5324

Merged
merged 1 commit into from
May 8, 2017

Conversation

FreeSlave
Copy link
Contributor

@FreeSlave FreeSlave commented Apr 5, 2017

Setting null as environment value leads to removing this variable
according to SetEnvironmentVariable documentation while Posix leaves
unspecified what setenev should do if value is null.

@FreeSlave FreeSlave changed the title Make behavior of environment.opIndexAssign on Posix to match the beha… Make behavior of environment.opIndexAssign on Posix to match the behavior on Windows Apr 5, 2017
Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a simple change and I don't have a strong opinion about this, but as no one has bother to look at this so far, here are a couple of comments:

according to SetEnvironmentVariable documentation while Posix leaves unspecified what setenev should do

For the lazy ones: https://msdn.microsoft.com/en-us/library/windows/desktop/ms686206(v=vs.85).aspx

=> The idea of unifying the behavior seems appealing, it comes with a very low cost and probably doesn't hurt anyone.

Has anyone a (strong) opinion about this?

@@ -3103,6 +3103,7 @@ static:
/**
Assigns the given $(D value) to the environment variable with the given
$(D name).
If $(D value) is null the variable is removed from environment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: null as its code as well

@JackStouffer
Copy link
Member

This is a bug fix, as current behavior on posix is sending a SIGTERM when null is set.

@wilzbach
Copy link
Member

wilzbach commented May 8, 2017

Vibe.d seems to have a transient error?

Received string message: Bye bye
Received iii: 1 2 3
Receive loop finished.
Receiver watchdog failure.
uncaught exception
dwarfeh(224) fatal error
Program exited with code -6
script returned exit code 2

Does the new Project Tester already support restarting builds or do we need to kick him manually with a git rebase?

@JackStouffer
Copy link
Member

I don't see a way to restart it.

@MartinNowak Can return Jenkins to not required for merging until it's more stable?

…vior

on Windows.

Setting null as environment value leads to removing this variable
according to SetEnvironmentVariable documentation while Posix leaves
unspecified what setenev should do if value is null.
@wilzbach
Copy link
Member

wilzbach commented May 8, 2017

I don't see a way to restart it.

I rebased the PR manually, s.t. hopefully it can be auto-merged.

@dlang-bot dlang-bot merged commit d052675 into dlang:master May 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants