-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Start factoring out Unix-assuming code #10364
Conversation
c2aecc9
to
edfc808
Compare
unsetenv(name.first.c_str()); | ||
} | ||
|
||
void replaceEnv(const std::map<std::string, std::string> & newEnv) |
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.
I think this function is only used in processes.cc
so I think it might even make more sense to just put it in that file.
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.
I think I want to keep it here because of where it is declared, for now, but yes it does seem that all the env var modification stuff (clearEnv
too) is just used for process spawning. Once we get that working on Windows it would be good time to revisit this :).
20c3865
to
83571d3
Compare
83571d3
to
f42eca1
Compare
In the Nix commit, platform-specific sources will go here.
f42eca1
to
c9112b1
Compare
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.
Should be fine, except for the abbreviated identifier.
This splits files and adds new identifiers in preperation for supporting windows, but no Windows-specific code is actually added yet. Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
5851a6c
to
02fa206
Compare
Ehm, some of these moves don't make sense to me, in particular |
@edolstra Those are temporary moves until more is implemented, after which I'll move them back. (The next PR has comments on which |
Motivation
This splits files and adds new identifiers in preparation for supporting
windows, but no Windows-specific code is actually added yet.
Context
#8901 will add CPP for Windows, but not have to split up files because this PR does that. That will make that PR's diff easier to read.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.