-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: Use package.links
trick for Protobuf compilation
#32
feat: Use package.links
trick for Protobuf compilation
#32
Conversation
|
||
impl Descriptor { | ||
/// Constructs a descriptor and adds it to the global pool. | ||
pub fn new(_dependencies: &[&'static Self], descriptor_bytes: &[u8]) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if unused, please remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having dependency refs allows ensuring via type system that these dependencies are added in the global pool. Would it be better to force dependencies to get initialized via Lazy::force()
, or in some other way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type system doesn't ensure any relation between the dependencies and the newly constructed descriptor, so this is as good as Lazy::force (or just dereferencing the lazy descriptors).
/// Validates this configuration. | ||
fn validate(&self) -> anyhow::Result<()> { | ||
/// Flag set to `true` if a public compilation target was encountered in a build script. | ||
static HAS_PUBLIC_TARGET: AtomicBool = AtomicBool::new(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be hard to lift this constraint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically, it can be lifted by changing the exported variable name (manifest
rn) depending on the compilation target; but I didn't come up with a good way to ID compilation targets and passing these IDs to dependent crates. IMO, restricting a crate to at most one public target makes sense – it makes sure that all public generated code is in a single place. Do you have use cases for multiple public targets in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example roles crate could have a separate protobuf targets for validator and node roles. I don't see any large benefits though, so it can wait.
I should've said from the start: I'm not entirely sure that this approach is better than the one implemented currently.
Global descriptor pool management could theoretically be simplified even further by making
The current approach sidesteps both of these issues, and ensures topological sorting of descriptors via Rust type system, which IMO is quite cool. |
What ❔
Propagates Protobuf information via env variables by relying on package.links.
Why ❔
This simplifies Protobuf configuration and eliminate misconfiguration risks.