-
-
Notifications
You must be signed in to change notification settings - Fork 641
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
fix Issue 18729 - dmd -run executes in different environment #8121
Conversation
Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#8121" |
2e6a314
to
bbbd94f
Compare
f22f3bd
to
6339f5a
Compare
462442a
to
9ea16d0
Compare
88fbcec
to
10c1a94
Compare
src/dmd/env.d
Outdated
return; // already saved | ||
} | ||
originalVars.push(LocalEnvVar.xmalloc(name, | ||
name.toCStringThen!(n => getenv(n.ptr)).toDString)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a lot of converting char*
to []
and back again. Wouldn't it be simpler to just leave them as char*
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a lot of converting char* to [] and back again
Alot? This is the one and only place in this change that we're calling toCStringThen
. And we're not converting name
back to a DString, we're converting the return value of getenv
to a DString.
We're using the char[]
version of name
for both the comparison and to pass to the LocalEnvVar
allocation function. We only need the char*
version to call getenv
. I'm not sure what you'd like me to change here.
@WalterBright I've integrated all your suggestions except the one about |
d20cc93
to
3d85f04
Compare
Thank you. In thinking about it some more, what's happening is simply storing
Use the |
47b0b0e
to
9a6975b
Compare
Ok, I've changed the code per your suggestions. |
Fix issue 18729
Much better. |
Thanks alot for guiding this through to the finish line. |
TLDR: the following fails on windows
I ran into this problem when I was running a D program via
dmd -run
that itself invoked DMD. The resulting program was invoking the compiler with-m64
, but the originaldmd -run
did not use-m64
. This caused a problem because the first invocation of the compiler set theLINKCMD
environment variable to the OPTLINK executable, which caused the second invocation to use the OPTLINK executable but it treated it like the MSVC linker because it was running in 64-bit mode.The solution here is to maintain the original environment (the environment before it was changed by the compiler) when
dmd -run
executes the compiled program. This PR accomplishes this by saving the original values of environment variables when they are overwritten and then restoring these values before executing the compiled program.