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

Start factoring out Unix-assuming code #10364

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

Ericson2314
Copy link
Member

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.

@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Mar 29, 2024
@github-actions github-actions bot added the new-cli Relating to the "nix" command label Mar 29, 2024
@Ericson2314 Ericson2314 changed the title Start factoring out Unix assumptions Start factoring out Unix-assuming code Mar 29, 2024
unsetenv(name.first.c_str());
}

void replaceEnv(const std::map<std::string, std::string> & newEnv)
Copy link
Contributor

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.

Copy link
Member Author

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 :).

In the Nix commit, platform-specific sources will go here.
Copy link
Member

@roberth roberth left a 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.

src/libutil/file-descriptor.hh Outdated Show resolved Hide resolved
src/libutil/file-descriptor.hh Outdated Show resolved Hide resolved
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 <[email protected]>
@Ericson2314 Ericson2314 merged commit 478c053 into NixOS:master Apr 2, 2024
9 checks passed
@Ericson2314 Ericson2314 deleted the split-out-unix branch April 2, 2024 19:09
@edolstra
Copy link
Member

edolstra commented Apr 3, 2024

Ehm, some of these moves don't make sense to me, in particular src/nix/unix/{fmt.*,run.*,upgrade-nix.*} are not Unix-specific.

@Ericson2314
Copy link
Member Author

@edolstra Those are temporary moves until more is implemented, after which I'll move them back.

(The next PR has comments on which #ifdefs are "temporary" (due to something being not yet implemented) or "permanent", but it is less clear to me how best to indicate that on whole files.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command windows
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants