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

Improve protocol major macro #7523

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

roberth
Copy link
Member

@roberth roberth commented Dec 28, 2022

Why

What

Most significant change: add PROTOCOL_MAJOR. All other changes follow from this. From the commit message:

PROTOCOL_MAJOR returns a simple major number, rather than the
number multiplied by 256. This is what one would reasonably
expect from GET_PROTOCOL_MAJOR, but we can't just change a
macro in a public header; hence the "rename".

PROTOCOL_MAJOR returns a simple major number, rather than the
number multiplied by 256. This is what one would reasonably
expect from GET_PROTOCOL_MAJOR, but we can't just change a
macro in a public header; hence the "rename".
PROTOCOL_MAJOR returns a simple major number, rather than the
number multiplied by 256. This is what one would reasonably
expect from GET_PROTOCOL_MAJOR, but we can't just change a
macro in a public header; hence the "rename".
@roberth roberth added the contributor-experience Developer experience for Nix contributors label Dec 28, 2022
@@ -6,6 +6,9 @@ namespace nix {
#define SERVE_MAGIC_2 0x5452eecb

#define SERVE_PROTOCOL_VERSION (2 << 8 | 7)
#define PROTOCOL_MAJOR(x) (((x) & 0xff00) >> 8)
#define PROTOCOL_MINOR(x) ((x) & 0x00ff)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does anything speak against implementing that as a nicely typed function like this:

constexpr size_t protocol_minor(size_t x) { return x & 0xff; }

This way this happens at compile time if possible and no one will ever have to handle problems that occur from entering the wrong signed/unsigned type.

@@ -10,6 +10,9 @@ namespace nix {
#define WORKER_MAGIC_2 0x6478696f

#define PROTOCOL_VERSION (1 << 8 | 34)
#define PROTOCOL_MAJOR(x) (((x) & 0xff00) >> 8)
#define PROTOCOL_MINOR(x) ((x) & 0x00ff)
Copy link
Contributor

@tfc tfc Dec 31, 2022

Choose a reason for hiding this comment

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

it seems like it would make sense to give the different major/minor macros (or functions if the other suggestion is followed) differing names as this also makes it easier to grep for their usage etc.

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Duh, that was indeed a nice footgun.

In addition to #7523 (comment), could we also deprecate the old macros? Deprecating a macro isn't really possible as far as I know, but we can hack something like:

constexpr size_t deprecated(size_t x) { return x }
#define GET_PROTOCOL_MAJOR(x) (deprecated((x) & 0xff00))
#define GET_PROTOCOL_MINOR(x) (deprecated((x) & 0x00ff))

@edolstra
Copy link
Member

edolstra commented Jan 2, 2023

I would just remove the old macros. I think the only use is in Hydra which can be updated easily.

@Ericson2314 Ericson2314 self-assigned this Feb 13, 2023
@roberth
Copy link
Member Author

roberth commented Feb 13, 2023

The old macro should stick around for compatibility, as Hydra isn't the only consumer. The new macros are also necessary for library-consuming code that needs to work around certain issues (shouldn't have to, but escape hatches are good).

TODO: clean up the implementation code to use a struct version.minor is nicer than a ton of macro calls. (thanks @Ericson2314)

@Ericson2314
Copy link
Member

@roberth and I discussed this, and we think it would be good for the C++ code to fully parse the version once into a struct, and then pass that around instead.

@roberth also mentioned it would still be good to have a macro to --- in effect --- document the binary format itself separate from Nix's parser implementing it (which is the spirit of these foo-protocol.hh headers anyways), and that sounds fine to me.

@roberth
Copy link
Member Author

roberth commented Feb 28, 2023

fully parse the version once into a struct

Have tried this but failed. I accidentally broke something.

I would prefer to just merge this, to provide an alternative to the footgun, and then move on to some more relevant problem solving.

@Ericson2314
Copy link
Member

@roberth Sounds good. Do you think you could push your attempt somewhere? I would be happy to take a stab at the follow-up if that helps.

@roberth
Copy link
Member Author

roberth commented Feb 28, 2023

It looks like I did a proper rage quit deleting my edits. It's mostly just a bunch of find and replace, so it's probably best to start over anyway. Sorry for dropping the actual struct bit though :/

EDIT: also this was like a week ago and my reflog is not helping either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants