Make a few changes in prepartion for deeper cleanup of the remote protocols#8547
Merged
roberth merged 5 commits intoNixOS:masterfrom Jun 19, 2023
Merged
Make a few changes in prepartion for deeper cleanup of the remote protocols#8547roberth merged 5 commits intoNixOS:masterfrom
roberth merged 5 commits intoNixOS:masterfrom
Conversation
This is generally a fine practice: Putting implementations in headers makes them harder to read and slows compilation. Unfortunately it is necessary for templates, but we can ameliorate that by putting them in a separate header. Only files which need to instantiate those templates will need to include the header with the implementation; the rest can just include the declaration. This is now documenting in the contributing guide. Also, it just happens that these polymorphic serializers are the protocol agnostic ones. (Worker and serve protocol have the same logic for these container types.) This means by doing this general template cleanup, we are also getting a head start on better indicating which code is protocol-specific and which code is shared between protocols.
Ericson2314
requested review from
edolstra,
fricklerhandwerk and
thufschmitt
as code owners
github-actions
bot
added
new-cli
Ericson2314
commented
Jun 19, 2023
Ericson2314
force-pushed
the
proto-cleanup-prep
branch
from
dff1846 to
49400dd
Compare
See API docs on that struct for why. The pasing as as template argument doesn't yet happen in that commit, but will instead happen in later commit. Also make `WorkerOp` (now `Op`) and enum struct. This led us to catch that two operations were not handled! Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
The motivation is exactly the same as for the last commit. In addition, this anticipates us formally defining separate serialisers for the serve protocol.
Pass this around instead of `Source &` and `Sink &` directly. This will give us something to put the protocol version on once the time comes. To do this ergonomically, we need to expose `RemoteStore::Connection`, so do that too. Give it some more API docs while we are at it.
Ericson2314
force-pushed
the
proto-cleanup-prep
branch
from
49400dd to
9f69b7d
Compare
Ericson2314
commented
Jun 19, 2023
Member
Author
Ericson2314
left a comment
Ericson2314
left a comment
There was a problem hiding this comment.
Noted items from #6223 now fixed here
| /** | ||
| * Convenience for debugging. | ||
| * | ||
| * @todo Perhaps render known opcodes more nicely. |
| * Convenience for debugging. | ||
| * | ||
| * We specialize the struct merely to indicate that we are implementing | ||
| * @todo Perhaps render known opcodes more nicely. |
Comment on lines
+82
to
+97
| // This is the definition of `Serialise` we *want* to put here, but | ||
| // do not do so. | ||
| // | ||
| // The problem is that if we do so, C++ will think we have | ||
| // seralisers for *all* types. We don't, of course, but that won't | ||
| // cause an error until link time. That makes for long debug cycles | ||
| // when there is a missing serialiser. | ||
| // | ||
| // By not defining it globally, and instead letting individual | ||
| // serialisers specialise the type, we get back the compile-time | ||
| // errors we would like. When no serialiser exists, C++ sees an | ||
| // abstract "incomplete" type with no definition, and any attempt to | ||
| // use `to` or `from` static methods is a compile-time error because | ||
| // they don't exist on an incomplete type. | ||
| // | ||
| // This makes for a quicker debug cycle, as desired. |
Member
Author
There was a problem hiding this comment.
Longer comment explaining what is going on.
These were never needed for this file, and date back to before this was split from `derivation-goal.cc`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
#6223 is approved pending some investigations of whether a few things can be done differently. This is the first few commits of that for which there were no major issues.
There is still a fair bit of code churn, however, so it is nice to merge this sooner to get that out of the way.
Context
#8365 and #8360 were previous rounds of cleanup that were also broken off and separated from #6223 to make things easier.
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.shsrc/*/teststests/nixos/*Priorities
Add 👍 to pull requests you find important.