Skip to content

Comments

Start factoring out Unix-assuming code#10364

Merged
Ericson2314 merged 2 commits intoNixOS:masterfrom
obsidiansystems:split-out-unix
Apr 2, 2024
Merged

Start factoring out Unix-assuming code#10364
Ericson2314 merged 2 commits intoNixOS:masterfrom
obsidiansystems:split-out-unix

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
Member

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

@Ericson2314 Ericson2314 force-pushed the split-out-unix branch 3 times, most recently from 20c3865 to 83571d3 Compare March 31, 2024 17:37
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.

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 enabled auto-merge April 2, 2024 18:44
@Ericson2314 Ericson2314 merged commit 478c053 into NixOS:master Apr 2, 2024
@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