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.

Sign up for GitHub

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

Jump to bottom

Begin factoring out the protocol code #1165

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Begin factoring out the protocol code #1165

merged 1 commit into from
Jan 22, 2024

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

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

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

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
1 check passed
@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