Combine `AbstractPos`, `PosAdapter`, and `Pos` #9634

9999years · 2023-12-18T21:17:21Z

Motivation

Two big changes:

Combined AbstractPos and PosAdapter into Pos.
Moved SourcePath into libutil.

These changes allow error.hh and error.cc to access source path and position information, which we can use to produce better error messages (for example, we could consider omitting filenames when two or more consecutive stack frames originate from the same file, as in #9623).

These changes are non-functional -- they do not change the behavior or user-interface of Nix.

All changes

Here's a list of all changes to types and methods in this PR:

Moved Pos from libexpr/nixexpr.hh to libutil/position.hh
- Deleted PosAdapter from libexpr/nixexpr.cc
- Deleted AbstractPos from libutil/error.hh
Moved SourcePath from libfetchers/input-accessor.hh to libutil/source-path.hh
Moved fetchToStore from SourcePath::fetchToStore and InputAccessor::fetchToStore to a new file libfetchers/fetch-to-store.hh
Moved InputAccessor from libfetchers to libutil
- A SourcePath is a CanonPath and an InputAccessor, so this needed to be in libutil
- Fortunately, with fetchToStore removed, the class definition is very small and doesn't introduce any additional dependencies to libutil.
Moved makeFileIngestionPrefix from libstore/content-address.hh to libstore/file-ingestion-method.hh
Moved FileIngestionMethod from libutil/file-content-address.hh to libstore/file-ingestion-method.hh

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

roberth · 2023-12-19T16:01:40Z

cc @pennae who have optimized Pos before and have an open PR about it. Do you think this is roughly a good direction?
(note that it's still draft; not asking for a full review or anything)

src/libutil/source-path.hh

pennae · 2023-12-20T18:23:35Z

cc @pennae who have optimized Pos before and have an open PR about it. Do you think this is roughly a good direction? (note that it's still draft; not asking for a full review or anything)

we haven't reviewed this fully, but it seems like a good idea. Pos and its (old) friends are outside of the optimized path, so performance impact should be small to nil. generally in favor :)

Ericson2314 · 2023-12-21T12:52:11Z

src/libutil/file-ingestion-method.cc

+
+namespace nix {
+
+std::string makeFileIngestionPrefix(FileIngestionMethod m)


I don't really want this sort of encoding thing to move from libstore. it is an arbitrary choice of the store layer, not a reusable mechanism. If anything, I want to move this stuff out of content-address.{cc,hh} and back down to the places where it is used (printing and parsing derivations, store paths) since it isn't really consistent between those two things.

It seems that InputAccessor would have to expose a fingerprint, so that the rest of fetchToStore can be moved back to libstore.

Agreed, let me see how much of this I can move back to libstore.

OK, I moved file-content-address.hh and file-ingestion-method.hh from libutil to libstore.

Sorry I was unclear. I do want the enums themselves to live in libutil. Those do belong with archive.hh and source-accessor.hh, which are also in libutil. What doesn't belong is just the prefix string bits. The string prefix bits are not intrinsic to these concepts, and just relate to downstream concerns like derivation aterm and narinfo CA field.

If like, perhaps take a stab at #9278 ? The point of that issue to was to make precisely this sort of thing clear:

"File System Object" is a barely nix-specific notation, and that can stay in libutil (originally I proposed it go in a new library below libstore also.

"Store Object" is a very much nix-specific concept that should stay in libstore.

The prefix stuff in question have have a file system object name, but it is neither intrinsic to "file system object" or "store object". I should just push it down to the derivation / store path / narinfo serializations that need it. Serializations are intrinsic properties of data types being serialized, an indeed this colon-prefix stuff is no good and not not be used in the newer unstable JSON serializations. (It currently is, this needs to change.)

The short version I that you can leave things the way they were prior to this PR if you like, and I'll at some future point I'll move this serialization nonsense out of content-address.cc.

src/libutil/input-accessor.hh

src/libstore/file-ingestion-method.cc

9999years · 2023-12-26T05:42:52Z

@Ericson2314 Done, PTAL.

Ericson2314 · 2023-12-26T06:40:03Z

OK thank you very much @9999years!

9999years · 2023-12-27T17:48:47Z

@Ericson2314 Anything else left before merge?

Ericson2314 · 2024-01-02T20:09:57Z

Nothing more from me on that stuff, but sure I should read over the rest.

Ericson2314 · 2024-01-08T18:30:51Z

src/libutil/repair-flag.hh

I think this can be moved back, because fetchToStore didn't move.

Ericson2314 · 2024-01-08T18:32:30Z

@9999years when you rebased/force-pushed you (I assume accidentally) undid the changes you requested that you make (shrinking the diff).

Ericson2314 · 2024-01-08T18:33:59Z

https://github.com/NixOS/nix/compare/f6eca1696c98212879fa1ec9a84a5c985ce21b25..ee853597a8a8d2dcddf34bd347b2963ffe873206 this looks like the force pushed that force-push (second most recent) added-back the changes I requested you not do.

Also move `SourcePath` into `libutil`. These changes allow `error.hh` and `error.cc` to access source path and position information, which we can use to produce better error messages (for example, we could consider omitting filenames when two or more consecutive stack frames originate from the same file).

9999years · 2024-01-08T19:00:13Z

@Ericson2314 Nice catch, got that code restored correctly (I think).

Ericson2314

Thanks @9999years! Sorry for the delay in the final review.

…-pos Combine `AbstractPos`, `PosAdapter`, and `Pos` (cherry picked from commit 113499d) === this is a bit cursed because originally it was based on InputAccessor code that we don't have and moved/patched features we likewise don't have (fetchToStore caching, all the individual accessors, ContentAddressMethod). the commit is adjusted accordingly to match (remove caching, ignore accessors, use FileIngestionMethod). note that `state.rootPath . CanonPath == abs` and computeStorePathForPath works relative to cwd, so the slight rewrite in the moved fetchToStore is legal. Change-Id: I05fd340c273f0bcc8ffabfebdc4a88b98083bce5

github-actions bot added store Issues and pull requests concerning the Nix store repl The Read Eval Print Loop, "nix repl" command and debugger fetching Networking with the outside (non-Nix) world, input locking labels Dec 18, 2023

9999years mentioned this pull request Dec 18, 2023

Omit duplicate consecutive filenames in stack traces #9623

Closed

9999years force-pushed the combine-abstract-pos-and-pos branch from 11ce214 to 4ac4d0d Compare December 19, 2023 18:03

9999years marked this pull request as ready for review December 19, 2023 18:03

9999years requested review from thufschmitt, roberth and edolstra as code owners December 19, 2023 18:03

edolstra reviewed Dec 20, 2023

View reviewed changes

src/libutil/source-path.hh Show resolved Hide resolved

Ericson2314 reviewed Dec 21, 2023

View reviewed changes

9999years force-pushed the combine-abstract-pos-and-pos branch from 702b028 to 85d4a8d Compare December 21, 2023 19:33

9999years requested a review from Ericson2314 December 21, 2023 19:37

Ericson2314 reviewed Dec 22, 2023

View reviewed changes

src/libutil/input-accessor.hh Show resolved Hide resolved

9999years force-pushed the combine-abstract-pos-and-pos branch 3 times, most recently from 9b53d24 to a3caa8d Compare December 22, 2023 20:47

Ericson2314 reviewed Dec 26, 2023

View reviewed changes

src/libstore/file-ingestion-method.cc Outdated Show resolved Hide resolved

9999years force-pushed the combine-abstract-pos-and-pos branch from a3caa8d to f6eca16 Compare December 26, 2023 05:42

9999years requested a review from Ericson2314 December 26, 2023 05:42

9999years force-pushed the combine-abstract-pos-and-pos branch 2 times, most recently from ee85359 to e824cb8 Compare January 4, 2024 18:53

Ericson2314 self-assigned this Jan 8, 2024

Ericson2314 added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Jan 8, 2024

Ericson2314 reviewed Jan 8, 2024

View reviewed changes

src/libutil/repair-flag.hh Outdated

Copy link

Member

Ericson2314 Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be moved back, because fetchToStore didn't move.

9999years force-pushed the combine-abstract-pos-and-pos branch from e824cb8 to 4feb7d9 Compare January 8, 2024 18:59

9999years requested a review from Ericson2314 January 8, 2024 19:00

Ericson2314 approved these changes Jan 11, 2024

View reviewed changes

Ericson2314 merged commit 113499d into NixOS:master Jan 11, 2024
8 checks passed

9999years deleted the combine-abstract-pos-and-pos branch January 11, 2024 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine `AbstractPos`, `PosAdapter`, and `Pos` #9634

Combine `AbstractPos`, `PosAdapter`, and `Pos` #9634

9999years commented Dec 18, 2023 •

edited

Loading

roberth commented Dec 19, 2023

pennae commented Dec 20, 2023

Ericson2314 Dec 21, 2023 •

edited

Loading

roberth Dec 21, 2023

9999years Dec 21, 2023

9999years Dec 21, 2023

Ericson2314 Dec 22, 2023

Ericson2314 Dec 22, 2023

9999years commented Dec 26, 2023

Ericson2314 commented Dec 26, 2023

9999years commented Dec 27, 2023

Ericson2314 commented Jan 2, 2024

Ericson2314 Jan 8, 2024

Ericson2314 commented Jan 8, 2024 •

edited

Loading

Ericson2314 commented Jan 8, 2024

9999years commented Jan 8, 2024

Ericson2314 left a comment


		namespace nix {

		std::string makeFileIngestionPrefix(FileIngestionMethod m)

Combine AbstractPos, PosAdapter, and Pos #9634

Combine AbstractPos, PosAdapter, and Pos #9634

Conversation

9999years commented Dec 18, 2023 • edited Loading

Motivation

All changes

Priorities and Process

roberth commented Dec 19, 2023

pennae commented Dec 20, 2023

Ericson2314 Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

roberth Dec 21, 2023

Choose a reason for hiding this comment

9999years Dec 21, 2023

Choose a reason for hiding this comment

9999years Dec 21, 2023

Choose a reason for hiding this comment

Ericson2314 Dec 22, 2023

Choose a reason for hiding this comment

Ericson2314 Dec 22, 2023

Choose a reason for hiding this comment

9999years commented Dec 26, 2023

Ericson2314 commented Dec 26, 2023

9999years commented Dec 27, 2023

Ericson2314 commented Jan 2, 2024

Ericson2314 Jan 8, 2024

Choose a reason for hiding this comment

Ericson2314 commented Jan 8, 2024 • edited Loading

Ericson2314 commented Jan 8, 2024

9999years commented Jan 8, 2024

Ericson2314 left a comment

Choose a reason for hiding this comment

Combine `AbstractPos`, `PosAdapter`, and `Pos` #9634

Combine `AbstractPos`, `PosAdapter`, and `Pos` #9634

9999years commented Dec 18, 2023 •

edited

Loading

Ericson2314 Dec 21, 2023 •

edited

Loading

Ericson2314 commented Jan 8, 2024 •

edited

Loading