Keep splitting libcmd headers & files #7748

Ericson2314 · 2023-02-04T04:06:46Z

Motivation

More splitting things up for tidiness and/or NixOS/rfcs#134 sake.

Each commit can be understood in isolation:

Split out `InstallableFlake` and `InstallableAttrPath`

Picking up where #7744 left off.

Slight cleanup of `InstallablesCommand::load`

See comments below, just removing dead code and deduplicate branches.

Split out `CmdRepl` and `editorFor`

The REPL itself and the nix repl CLI are conceptually different things, and thus deserve to be in different files.

I also think libcmd should have the infrastructure for making the commands, but not the commands themselves. CmdRepl, previously there, not back inside the Nix binary with the other commands, would be one of those "commands themselves".

An existing "obvious home" for editorFor didn't pop into my mind, so I just gave it its own header and file.

Context

Depends on #7744, see the PR's context for additional information.

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
code and comments are self-explanatory
commit message explains why the change was made
new feature or bug fix: updated release notes

Ericson2314 · 2023-02-10T13:47:25Z

.gitignore

@@ -103,6 +103,7 @@ outputs/

 *.a
 *.o
+*.o.tmp


Oh oops sorry this snuck in here. Clang build temp files like this, which I noticed when I started developing with clang, and so I added this line while working. They are deleted by Clang after the build is complete, but if you run git status while a build is in progress (say in another terminal), they clutter the output.

I had meant to PR this separately; let me know if I still should.

dpulls · 2023-02-13T13:57:24Z

🎉 All dependencies have been resolved !

Ericson2314 · 2023-02-13T14:35:22Z

src/libcmd/installables.cc

@@ -1000,8 +715,8 @@ void InstallablesCommand::prepare()
    installables = load();
 }

-Installables InstallablesCommand::load() {
-    Installables installables;


This variable was unused; surprised there was no warning!

Ericson2314 · 2023-02-13T14:36:02Z

src/libcmd/installables.cc

-    if (_installables.empty()) {
-        if (useDefaultInstallables())
-            return {"."};
-        return {};


Since _installables is empty, this is the same as return _installables; below.

nixos-discourse · 2023-02-13T15:54:03Z

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

https://discourse.nixos.org/t/2023-02-10-nix-team-meeting-minutes-31/25438/1

Ericson2314 · 2023-02-17T13:10:37Z

src/libcmd/local.mk

@@ -6,7 +6,7 @@ libcmd_DIR := $(d)

 libcmd_SOURCES := $(wildcard $(d)/*.cc)

-libcmd_CXXFLAGS += -I src/libutil -I src/libstore -I src/libexpr -I src/libmain -I src/libfetchers -I src/nix


This layer violation is no longer needed after moving these things around.

The issue was the #include "repl.md", which was left behind in src/nix not moved to src/libcmd. It could of been moved over, but this PR solves it by moving (the CmdRepl part of) repl.cc back.

fricklerhandwerk · 2023-02-20T14:02:39Z

Discussed on the Nix team meeting:

@tomberek: lots of changes, would be comfortable with having more eyes on it

refactoring is just like transformation into a different coordinate frame where the desired user-facing change is small
@edolstra: move of the REPL into the header is questionable. the problem is, C++ being what it is, it leaks half of the implementation into the interface
- (some discussion about how to wire up the REPL)
- otherwise looks good
@edolstra: add an abstract REPL class to keep the header small

The REPL itself and the `nix repl` CLI are conceptually different things, and thus deserve to be in different files.

nixos-discourse · 2023-02-20T15:01:10Z

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

https://discourse.nixos.org/t/2023-02-20-nix-team-meeting-minutes-34/25625/1

edolstra · 2023-02-21T12:45:27Z

This broke the noGC builds: https://hydra.nixos.org/build/210107828

And the aarch64-linux build is segfaulting: https://hydra.nixos.org/build/210107827

tomberek · 2023-02-21T13:50:26Z

looking….

Ericson2314 · 2023-02-21T13:59:27Z

I know how to fix the first, I guess I will try to do a binfmt misc qemu GDB for the second.

Ericson2314 · 2023-02-21T14:55:50Z

The aarch64-linux failure is not due to this but instead #6538

nixos-discourse · 2023-04-21T11:58:00Z

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

https://discourse.nixos.org/t/nix-team-report-2022-10-2023-03/27486/1

Ericson2314 requested review from edolstra and thufschmitt as code owners February 4, 2023 04:06

Ericson2314 changed the title ~~Split other installables~~ Keep splitting libcmd headers & files Feb 4, 2023

Ericson2314 marked this pull request as draft February 4, 2023 04:07

Ericson2314 mentioned this pull request Feb 4, 2023

Make command infra less stateful and more regular #7750

Merged

7 tasks

thufschmitt assigned edolstra Feb 10, 2023

Ericson2314 assigned Ericson2314 and unassigned edolstra Feb 10, 2023

Ericson2314 commented Feb 10, 2023

View reviewed changes

Ericson2314 marked this pull request as ready for review February 13, 2023 13:57

Ericson2314 marked this pull request as draft February 13, 2023 13:57

Ericson2314 marked this pull request as ready for review February 13, 2023 13:58

Ericson2314 changed the base branch from master to 0.7-release February 13, 2023 14:05

Ericson2314 changed the base branch from 0.7-release to master February 13, 2023 14:05

Ericson2314 assigned tomberek and unassigned Ericson2314 Feb 13, 2023

Ericson2314 commented Feb 13, 2023

View reviewed changes

Ericson2314 commented Feb 17, 2023

View reviewed changes

Ericson2314 mentioned this pull request Feb 20, 2023

Tracking issue for RFC 134 #7868

Open

10 tasks

tomberek approved these changes Feb 20, 2023

View reviewed changes

fricklerhandwerk added the RFC Related to an accepted RFC label Feb 20, 2023

Ericson2314 added 3 commits February 20, 2023 09:09

Split out InstallableFlake and InstallableAttrPath

fa4733f

Slight cleanup of InstallablesCommand::load

57a2e46

Split out CmdRepl and editorFor

1bd03ad

The REPL itself and the `nix repl` CLI are conceptually different things, and thus deserve to be in different files.

Ericson2314 force-pushed the split-other-installables branch from 7a85980 to 1bd03ad Compare February 20, 2023 14:46

tomberek merged commit 924ef67 into NixOS:master Feb 20, 2023

Ericson2314 deleted the split-other-installables branch February 20, 2023 15:29

This was referenced Feb 21, 2023

Test everything before merging with merge queue + hercules-ci #7674

Open

Fix the build without GC #7874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep splitting libcmd headers & files #7748

Keep splitting libcmd headers & files #7748

Ericson2314 commented Feb 4, 2023 •

edited by tomberek

Loading

Ericson2314 Feb 10, 2023

dpulls bot commented Feb 13, 2023

Ericson2314 Feb 13, 2023

Ericson2314 Feb 13, 2023

nixos-discourse commented Feb 13, 2023

Ericson2314 Feb 17, 2023

Ericson2314 Feb 17, 2023 •

edited

Loading

fricklerhandwerk commented Feb 20, 2023 •

edited

Loading

nixos-discourse commented Feb 20, 2023

edolstra commented Feb 21, 2023

tomberek commented Feb 21, 2023

Ericson2314 commented Feb 21, 2023

Ericson2314 commented Feb 21, 2023

nixos-discourse commented Apr 21, 2023

@@ @@ -103,6 +103,7 @@ outputs/ @@
               *.a
               *.o
+              *.o.tmp

		@@ -6,7 +6,7 @@ libcmd_DIR := $(d)

		libcmd_SOURCES := $(wildcard $(d)/*.cc)

		libcmd_CXXFLAGS += -I src/libutil -I src/libstore -I src/libexpr -I src/libmain -I src/libfetchers -I src/nix

Keep splitting libcmd headers & files #7748

Keep splitting libcmd headers & files #7748

Conversation

Ericson2314 commented Feb 4, 2023 • edited by tomberek Loading

Motivation

Split out InstallableFlake and InstallableAttrPath

Slight cleanup of InstallablesCommand::load

Split out CmdRepl and editorFor

Context

Checklist for maintainers

Ericson2314 Feb 10, 2023

Choose a reason for hiding this comment

dpulls bot commented Feb 13, 2023

Ericson2314 Feb 13, 2023

Choose a reason for hiding this comment

Ericson2314 Feb 13, 2023

Choose a reason for hiding this comment

nixos-discourse commented Feb 13, 2023

Ericson2314 Feb 17, 2023

Choose a reason for hiding this comment

Ericson2314 Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

fricklerhandwerk commented Feb 20, 2023 • edited Loading

nixos-discourse commented Feb 20, 2023

edolstra commented Feb 21, 2023

tomberek commented Feb 21, 2023

Ericson2314 commented Feb 21, 2023

Ericson2314 commented Feb 21, 2023

nixos-discourse commented Apr 21, 2023

Ericson2314 commented Feb 4, 2023 •

edited by tomberek

Loading

Split out `InstallableFlake` and `InstallableAttrPath`

Slight cleanup of `InstallablesCommand::load`

Split out `CmdRepl` and `editorFor`

Ericson2314 Feb 17, 2023 •

edited

Loading

fricklerhandwerk commented Feb 20, 2023 •

edited

Loading