Split up `util.{hh,cc}` by Ericson2314 · Pull Request #8920 · NixOS/nix

Ericson2314 · 2023-09-04T19:06:28Z

Motivation

All OS and IO operations should be moved out, leaving only some misc portable pure functions.

This is useful to avoid copious CPP when doing things like Windows and Emscripten ports.

Context

This sort of thing is annoying to write and annoying to read, but I realized I have to do it before trying to finish #8901.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
documentation in the internal API docs
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

fricklerhandwerk · 2023-09-22T12:34:07Z

Triaged in Nix maintainers meeting:

@Ericson2314: goal is to get closer to make Nix work on Windows
@edolstra: would be better to make the changes when there is direct need
- you will only know exactly what the ideal factoring is once you start the porting
@tomberek: recently played around with providing an Emscripten build output, this would help with that kind of thing
@edolstra: two more problems:
- adds friction to using git blame
- more compilation units increases compilation time
  - @Ericson2314: would rather focus on readability, can always add compilation performance hacks on top (e.g. pre-concatenating files)
    - can do follow up and also merge into lazy trees
idea approved

nixos-discourse · 2023-09-22T13:51:51Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-09-22-nix-team-meeting-minutes-88/33343/1

fricklerhandwerk

Okay, I gave it an almost complete but still somewhat cursory look. I think the desired end result is very desirable.

I fully trust you on doing the right thing here, so that's approved.

Yet I'm quite concerned about this being a huge diff that could allow mistakes to slip through into foundational components, and I wouldn't want to share responsibility for that. I'd feel a lot more comfortable with doing that in multiple steps, even if that ends up being 30% more work. It will also ease dissection a lot.

Multiple commits may be fine, but reviews leading to rewriting history will just produce friction as those tend to confuse me, so I'd err towards multiple PRs. If anything, it will make a lot more likely for me to really go through each component and make sure I understand it and its role in the system, so I see it as a good opportunity for knowledge sharing. Being involved in way too many things, spending hours in a row staring at code is something I simply can't afford these days.

If anyone else on the @nixos/nix-team says "nah it'll be fine" we can of course just merge it as is.

fricklerhandwerk · 2023-11-05T12:35:06Z

src/libutil/ambient-authority.hh

That's a weird name. What is it really about? Why is it not e.g. system-specific.hh?

I went with current-process.hh, with the new first commit nativeSystem is no longer there, and everything else is squarely about the current process.

fricklerhandwerk · 2023-11-05T12:39:43Z

src/libutil/file-descriptor.cc

+
+std::string drainFD(int fd, bool block, const size_t reserveSize)
+{
+    // the parser needs two extra bytes to append terminating characters, other users will


Which parser?

I just moved that code :)

I don't think we need a CPP defininition and a header entry, and this way allows constant expression elimination.

All OS and IO operations should be moved out, leaving only some misc portable pure functions. This is useful to avoid copious CPP when doing things like Windows and Emscripten ports. Newly exposed functions to break cycles: - `restoreSignals` - `updateWindowSize`

Ericson2314 · 2023-11-05T17:31:28Z

Thanks @fricklerhandwerk. So one that makes me feel a lot better about this is git show --color-moved. That "proves" that this is just moving code around, and thus while it's a lot of code motion and the drudgery of cutting and pasting was mind-melting, there isn't anything interesting going on.

I do agree knowledge sharing is good, but I an easier way to do that is going through these files post split, e.g. during the Windows porting process. It's much easier to digest things when they are already cut into bite sized pieces :).

As you saying trying to redo this into separate PRs for each header would take longer. While I don't really mind it for the headers themselves, it would worse code churn on the #includes in all the other files and headers came and went. Because of that, because we have so many more interesting things to spend reviewer time on (this is a boring step towards an interesting destination, Windows), and because you are on board with breaking up the giant file, I think it is just best to merge this as-is.

roberth · 2023-11-05T20:53:07Z

These moves were long overdue. Merging while the merge base is up to date.

Not expecting much merge conflicts from this, as this is support code - not a place where features happen. Certainly we have better places than a "utils" file by now!

git show --color-moved.

This is a life saver for review.

The test split matches PR NixOS#8920, so the utility files and tests files are once again to 1-1. The string changes continues what was started in PR NixOS#11093.

Ericson2314 force-pushed the split-util-cchh branch from a23797e to 937168e Compare September 4, 2023 19:59

thufschmitt assigned fricklerhandwerk Sep 22, 2023

Ericson2314 added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Sep 22, 2023

Ericson2314 force-pushed the split-util-cchh branch from 937168e to f1fc3e2 Compare October 19, 2023 13:56

Ericson2314 marked this pull request as ready for review October 19, 2023 14:29

Ericson2314 requested review from edolstra and thufschmitt as code owners October 19, 2023 14:29

Ericson2314 force-pushed the split-util-cchh branch 8 times, most recently from 3875194 to 393d16f Compare October 20, 2023 05:20

Ericson2314 mentioned this pull request Oct 21, 2023

Build a minimized Nix with MinGW #8901

Merged

8 tasks

Ericson2314 force-pushed the split-util-cchh branch from 393d16f to a960b53 Compare October 21, 2023 17:37

Ericson2314 added the windows label Oct 22, 2023

Ericson2314 force-pushed the split-util-cchh branch from a960b53 to 3130dbc Compare October 25, 2023 04:43

Ericson2314 force-pushed the split-util-cchh branch from 3130dbc to fd9b701 Compare November 4, 2023 19:52

fricklerhandwerk approved these changes Nov 5, 2023

View reviewed changes

Ericson2314 added 2 commits November 5, 2023 12:18

Narrower scope for nativeSystem

2678b51

I don't think we need a CPP defininition and a header entry, and this way allows constant expression elimination.

Split up util.{hh,cc}

ac89bb0

All OS and IO operations should be moved out, leaving only some misc portable pure functions. This is useful to avoid copious CPP when doing things like Windows and Emscripten ports. Newly exposed functions to break cycles: - `restoreSignals` - `updateWindowSize`

Ericson2314 force-pushed the split-util-cchh branch from fd9b701 to ac89bb0 Compare November 5, 2023 17:20

roberth approved these changes Nov 5, 2023

View reviewed changes

roberth merged commit 1a14ce8 into NixOS:master Nov 5, 2023

Ericson2314 deleted the split-util-cchh branch November 5, 2023 22:07

roberth mentioned this pull request Nov 6, 2023

Util improvements #9279

Merged

Ericson2314 mentioned this pull request Aug 5, 2024

Split util tests, organize more string functions #11256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Split up `util.{hh,cc}`#8920

Split up `util.{hh,cc}`#8920
roberth merged 2 commits intoNixOS:masterfrom
obsidiansystems:split-util-cchh

Ericson2314 commented Sep 4, 2023 •

edited

Loading

Uh oh!

fricklerhandwerk commented Sep 22, 2023

Uh oh!

nixos-discourse commented Sep 22, 2023

Uh oh!

fricklerhandwerk left a comment •

edited

Loading

Uh oh!

fricklerhandwerk Nov 5, 2023

Uh oh!

Ericson2314 Nov 5, 2023

Uh oh!

fricklerhandwerk Nov 5, 2023

Uh oh!

Ericson2314 Nov 5, 2023

Uh oh!

Ericson2314 commented Nov 5, 2023

Uh oh!

roberth commented Nov 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Comments

Conversation

Ericson2314 commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Checklist for maintainers

Priorities

Uh oh!

fricklerhandwerk commented Sep 22, 2023

Uh oh!

nixos-discourse commented Sep 22, 2023

Uh oh!

fricklerhandwerk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fricklerhandwerk Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

fricklerhandwerk Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Nov 5, 2023

Uh oh!

roberth commented Nov 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ericson2314 commented Sep 4, 2023 •

edited

Loading

fricklerhandwerk left a comment •

edited

Loading