Skip to content

Comments

Begin factoring out the protocol code#1165

Merged
Ericson2314 merged 1 commit intoNixOS:nix-nextfrom
obsidiansystems:factor-out-proto
Jan 22, 2024
Merged

Begin factoring out the protocol code#1165
Ericson2314 merged 1 commit intoNixOS:nix-nextfrom
obsidiansystems:factor-out-proto

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Feb 20, 2022 •
edited
Loading

After a few preparatory moves, this does cmdQueryValidPaths

See each commit for details and easier reviewing.

Progress towards #1164

This targets nix-next not master, so it is fine that this depends on an unreleased version of Nix. Only the Nix PR needs to be merged first.

Depends on NixOS/nix#6134
Depends on #1324

@Ericson2314 Ericson2314 mentioned this pull request Feb 20, 2022
Open
13 tasks
@edolstra
Copy link
Member

edolstra commented Feb 22, 2022

I think it's better to start using the Store abstraction. I started work on this a while ago, here is how far I got: https://pastebin.com/SA5TStt9. The remaining problems are lack of cancellation and some issues with logging.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Feb 22, 2022 •
edited
Loading

@edolstra I agree in principle, but I am very afraid of making too many changes too quickly.

With this approach, I can and preserve things like the fact the the hydra queryPathInfo locks the paths while the LegacySSHStore one doesn't --- if I hadn't work so piecemeal I might not have noticed this!

Long term, yes, I absolutely do want to use the store abstraction! But I want to get there very incrementally to avoid any regressions.

@Ericson2314 Ericson2314 mentioned this pull request Feb 22, 2022
Merged
@Ericson2314 Ericson2314 changed the title Begin factoring out the protocol code --- contains #1162 Begin factoring out the protocol code Nov 30, 2023
@Ericson2314 Ericson2314 marked this pull request as draft November 30, 2023 16:06
@Ericson2314 Ericson2314 changed the base branch from master to nix-next December 7, 2023 18:53
@Ericson2314
Copy link
Member Author

Ericson2314 commented Dec 7, 2023

It's passing locally! (CI should soon catch up :))

Ericson2314
Ericson2314 commented Dec 7, 2023
View reviewed changes
src/hydra-queue-runner/build-remote.cc Outdated
Comment on lines 218 to 220

// Do not attempt to speak a newer version of the protocol
conn.remoteVersion = std::min(conn.remoteVersion, our_version);
Copy link
Member Author

@Ericson2314 Ericson2314 Dec 7, 2023

Choose a reason for hiding this comment

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

I am shocked we got away with not having this...ever! If the two sides don't agree on a version, how can anything work?!

@Ericson2314 Ericson2314 force-pushed the factor-out-proto branch 2 times, most recently from c07679b to d972791 Compare December 11, 2023 15:47
@Ericson2314
Use nix::serv_proto::BasicConnection in build_remote.cc
4ac31c8 
- Use the type itself

  This lays the foundation for being able to dedup the protocol code.

- Use `BasicConnection::handshake`, replacing ours.

- Use `BasicConnection::queryValidPaths`

- Use `BasicConnection::putBuildDerivationRequest`
@Ericson2314 Ericson2314 marked this pull request as ready for review January 22, 2024 19:22
@Ericson2314 Ericson2314 merged commit 34c51fc into NixOS:nix-next Jan 22, 2024
@Ericson2314 Ericson2314 deleted the factor-out-proto branch January 22, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Ericson2314 @edolstra