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

Reorganize this repo #7876

Open
Ericson2314 opened this issue Feb 21, 2023 · 15 comments
Open

Reorganize this repo #7876

Ericson2314 opened this issue Feb 21, 2023 · 15 comments
Labels
build-problem Nix fails to compile or test; also improvements to build process contributor-experience Developer experience for Nix contributors idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome.

Comments

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 21, 2023

Desiderata

Here are a number of goals we would like to have, where the current organization falls shorts

Low latency Nix builds

For CI and local development a like, it is nice if repeated Nix builds avoid duplicated work. This means having fine-grained derivations with well-scoped inputs and outputs.

Ultimately, we would need Recursive Nix or RFC 92 to really nail this, but in the meantime we can at least focus on some lower hanging fruit: filtering sources just to the files relevant for the task at hand.

One way to do this is with builtins.filterSource and friends, but keeping such predicates up to date is a bit frustrating. A more maintainable way is to leverage the repo directory structure so that separate build tasks correspond to separate directories.

This also makes it easy to understand the project structure at a glance.

Cross-project incremental development.

For proper builds, dependencies should always be built in separate derivations, for the reason of low latency defined above. But for the foreseeable future, developers wanted the best developer experience will still be doing impure builds in a development shell environment.

If the developer is just working on one project, giving that shell environment the same dependencies as the regular build is fine, but quite often the developer wants to work on multiple projects at once. Then, having the dependencies be pre-built in the development shell is very frustrating for two reasons:

  • The developer must re-enter the shell every time they modify the dependency
  • The developer must way for a from-scratch rebuild of the dependency when they renter the development shell.

This is an issue both within this repo and between repos:

  • When trying to fix the Perl bindings (in tree) after a modification of Nix, one needs to wait for a from-scratch Nix rebuiild.
  • The Python bindings from Experimental python bindings #7735 (in tree), have a best-effort mitigation of this, but it is not easy to write today.
  • Developing Hydra (out of tree) also requires a from-scratch Nix rebuild. This makes solving issue Deduplicate code between Hydra and Nix hydra#1164, and is probably a reason why some of this duplication arose in the first place --- it is just too painful making small changes to Nix and easier to duplicate functionality in Hydra.

The relationship to repo organization is that in order to maintainably support both the development and release builds workflows, they must be as similar as possible. The means, the location of source files in the repo need to roughly correspond to the location of installed files in dependencies' outputs.

  • Headers should "look the same" both installed (with -I /nix/store/... flags) or in tree (with `-I this/repo/...) flags.
  • No -include config.h should be done, because downstream projects should need to include headers not replicate such CLI flags.

The status quo

Today, we have a directory structure like this:

flake.nix
perl/
├── lib
│   └── Nix
│       ├── Store.pm
│       ├── Store.xs
│       ...
...

python/
├── ...
...

Makefile

src
├── libcmd
│   ├── command.cc
│   ├── command.hh
│   ...
├── libexpr
│   ├── attr-path.cc
│   ├── attr-path.hh
│   ├── tests/
│   │    ├── value/context.hh
│   │    ├── value/context.cc
│   │    ...
│   ...
...

Here are the problems with this:

  • Even though we have separate top-level projects, Nix itself is splatted on the top level

    • The flakes.nix and perl binding sources thus pollute the build of Nix itself --- changing just the perl bindigns also invalidates the dev shell (so the developer must be careful not to leave the dev shell by mistake, or they will have to wait to reenter it!)
  • The header are directly in the root of -I search path entries, but they will be installed in $dev/include/nix/ -- a new nix subdir! We have a non-standard pkg-config hacking around this, but this is not a good solution.

  • The flake.nix is too big and hard to read.

  • The test headers are mix in with the library headers, and we have to manually be careful not to install them.

A plan

I propose we instead adopt something like this:

perl/
├── default.nix
├── lib
│   └── Nix
│       ├── Store.pm
│       ├── Store.xs
│       ...
...

python/
├── default.nix
├── ...
...

nix
├── default.nix
├── Makefile
├── libcmd
│   ├── include/nix/cmd
│   │   ├── command.hh
│   │   ...
│   └── src
│       ├── command.cc
│       ...
├── libexpr
│   ├── include/nix/expr
│   │   ├── attr-path.hh
│   │   ...
│   └── src
│       ├── attr-path.cc
│       ...
├── libexpr-tests
│   ├── include/nix/expr/tests/
│   │   ├── value/context.hh
│   │   │
│   │   ...
│   └── src
│       ├── value/context.hh
│       ...
...

integration-tests
├── authorization.nix
...

Note these changes:

  • Each project (Nix itself, Perl bindings, Python bindings) is confined to its own subdir, clarifying the project structure and avoiding input leakage for lower latency builds

  • Each project gets its own default.nix, which will be callPackaged in the top-level flake.nix. This makes clear what dependencies of each "unit" are, and those files could someday be mirrored one-for-one in Nixpkgs easing maintenance. The top-level flake.nix is much shorter and clearer.

    • We might someday use sub-flakes for this, but in my opinion they are not yet mature enough.
  • The extra src dir is removed, we can go straight to talking about each library

    • This helps cut down on the extra subdir cost we are paying.
    • Instead we have a src dir per library
  • The public headers are put in their own directory, and given the same prefix they would be installed under.

    • N.B. Putting the library name in the prefix (nix/ vs nix/expr, for example, is an orthogonal choice we can decide on separately.
    • If we ever had private headers (perhaps useful if we want to ever move towards a partially stable C++ interface), those would also go in the src directory, indicating they are not installed.
  • The test libraries are completely separated out

    • There is no risk of mixing up test srcs and headers for library proper srcs and headers
  • The integration tests (NixOS VM tests) are separated out, so that changes to them don't need to cause a nix rebuild

Bonus: Meson support

One of Meson's best features is Subprojects. This is designed to agglomerate a bunch of separate packages into one single incremental build with minimal hassle. Dependencies (or rather virtual things to depend-upon) can be declared in individual meson projects, and then when the projects are combined together needed-dependencies can be satisfied with these instead of externally (e.g. with `pkg-config).

If we switched to using Meson across the board (Nix itself, Python bindings, Perl bindings, Hydra), we would have a meson.build in each sub-directory, and then a top-level meson.build in the root of this repo adding all the others as sub-projects. The top-level meson.build would just be used for development.

If a developer wants to develop Nix and hydra at the same time, they would unpack/symlink hydra to a new subdir along-side the others and then add it with subproject(...) in the top-level meson.build. Then incremental builds of Nix and Hydra together would just work --- no further manual steps needed!

Drawbacks

  • A lot of files will move around

    • But this is a one time cost. Rebasing the PR that moves the files is hard, so we should decide on this issue first and then quickly implement whatever we decide on right after, but rebasing other PRs over that PR is easy.
  • File paths are longer and more redundant

    • True. Cutting out the src directory helps a bit, but we still have things like nix/libstore/include/nix/store/foo-bar.hh. But it doesn't seem that much can be done out of this without failing to meet our desiderata.
  • Files are further apart. foo.cc is no longer right next to foo.hh

    • True. But again, it doesn't seem that much can be done out of this without failing to meet our desiderata.
@Ericson2314 Ericson2314 changed the title WIP: Reorganizing this repo Reorganize this repo Feb 21, 2023
@Ericson2314 Ericson2314 added the contributor-experience Developer experience for Nix contributors label Feb 21, 2023
@roberth roberth added this to Nix team Feb 22, 2023
@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Feb 22, 2023

Overall this sounds reasonable to me except the navigability issues introduced by encoding the header install targets as paths:

we still have things like nix/libstore/include/nix/store/foo-bar.hh. But it doesn't seem that much can be done out of this without failing to meet our desiderata.

Alternative:

  • /
    • libstore
      • include
        • store
          • foo.hh
      • src
        • foo.cc

...that is, make all Nix components top-level, and set the complete install targets (e.g. /include/nix/store/) in the build system.

If there is generally only one build target per component, one may as well leave out the nested items within include.

@Ericson2314
Copy link
Member Author

@fricklerhandwerk The problem with that is that -I always "mounts" the item at the search path root, so one would do #include "store/foo.hh" in tree, but #include "nix/store/foo.hh" with the installed version.

I would love to go add an --include-prefixing flag to GCC and Clang, so we could do --include-prefixing nix/store $REPO_ROOT/libstore/include, but until then I don't see what choice we have but to duplicate the entire prefix we wish to install under in the source tree.

@fricklerhandwerk
Copy link
Contributor

I see. Now also convinced this is the least bad option.

@thufschmitt thufschmitt moved this to ⚖ To discuss in Nix team Feb 24, 2023
@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Feb 24, 2023

Triaged in the Nix team meeting:

  • @edolstra: this is what nix develop --redirect is for, we just have to document it better
  • @Ericson2314: having a non-standard pkg-config setup works somewhat but is really confusing
  • to discuss

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-02-24-nix-team-meeting-minutes-35/25757/1

@roberth
Copy link
Member

roberth commented Feb 26, 2023

Here's another repo-wide change we've been postponing.

Do we have more such changes to consider?

I've edited the proposal to add a better location for:

@fricklerhandwerk
Copy link
Contributor

Discussed in the Nix team meeting:

  • @Ericson2314 walked the team through the proposal
  • @thufschmitt: we may want to think about the independent changes separately
    • @Ericson2314: we could do everything except the other PR:
    • should have all changes that may cause merge conflicts or breakages separate from the others, and start with the second part
  • @edolstra: not super convinced about the usefulness
    • the only other project in the repo is the Perl code
    • @roberth: there are still the tests which are a pain to run
      • @edolstra: could also be done with builtins.filterSource
        • @thufschmitt: there was a PR merged recently doing that, but it's clunkier
    • @Ericson2314: a directory structure is easier to understand than a complicated set of filters
      • @edolstra: agreed, but only if it had been done this way to begin with
        • now it's a constly change
        • @Ericson2314: disagree, Git can handle that as well
          • @roberth: if content change and renames are different commits, it should work well
  • @edolstra: shouldn't we use subflakes?
    • @Ericson2314: subflakes are not well-formed yet, rather use something stable close to what Nixpkgs is doing with callPackage
  • @thufschmitt: would be fine with starting with tests and splitting Nix expressions for build components as a logical separation
  • agreement: move the tests as a first step, discuss the rest later

@thufschmitt thufschmitt removed this from Nix team Mar 27, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-03-27-nix-team-meeting-minutes-44/26759/1

@thufschmitt thufschmitt added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Apr 14, 2023
@thufschmitt thufschmitt moved this to ⚖ To discuss in Nix team Apr 14, 2023
@fricklerhandwerk
Copy link
Contributor

Discussed in Nix team meeting:

  • @thufschmitt: since we want to do at least two things at once (reorganise files to be more accessible for interfacing from the outside, and optimise the build process) we have to figure out what's the best way to do it
  • because of a Hydra bug due to it running the source tree branch, filterSource is unreliable
  • A prior attempt of moving files around: Run test suite on NixOS #7778
  • @Ericson2314: have to strike a balance between machine resource use and developer time
  • needs more discussion

@roberth
Copy link
Member

roberth commented Apr 16, 2023

I wish we could stop making excuses and workarounds, and just do the damn thing.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-04-14-nix-team-meeting-minutes-48/27358/1

@fricklerhandwerk fricklerhandwerk added idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. and removed idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. labels Jul 21, 2023
@fricklerhandwerk
Copy link
Contributor

Revisited in the Nix maintainer meeting:

@roberth will make a PR to update the source filter in flake.nix. A large refactoring is currently not practical, as we have hundreds of PRs many of which will likely get into conflicts. We want to do it in the long run though.

@Ericson2314 argues many files can be moved piecemeal, leveraging Git's support for renames. A careful attempt is encouraged. Also discussed #7847

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-07-21-nix-team-meeting-minutes-73/30768/1

@fricklerhandwerk
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk added the build-problem Nix fails to compile or test; also improvements to build process label Jan 21, 2024
@fricklerhandwerk
Copy link
Contributor

Related: #2503

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-problem Nix fails to compile or test; also improvements to build process contributor-experience Developer experience for Nix contributors idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome.
Projects
None yet
Development

No branches or pull requests

5 participants