Skip to content

Conversation

@Bromeon
Copy link
Contributor

@Bromeon Bromeon commented May 22, 2020

Hello! There seems to be no current API for selecting the capnp executable that is used to run the schema generation. Instead, the CompilerCommand.run() method assumes a capnp name is visible either relative to the Rust working directory, or in PATH. This disallows use cases such as shipping the capnp executable in a custom directory within the project.

I added the following method to CompilerCommand:

pub fn run_with<P>(&mut self, executable: P) -> ::capnp::Result<()>
where
    P: AsRef<Path>

which allows build.rs configurations with custom paths, such as:

capnpc::CompilerCommand::new()
    .file("src/schema/Point.capnp")
    .run_with("tools/external/capnp.exe")
    .expect("compiling schema");

The 2nd commit fixes some warnings regarding explicit lifetime annotations, which can be elided. Let me know if you prefer keeping those, and I'll remove the commit.


Maybe a general question related to the capnp generator: what is considered best practice for a self-contained and portable Rust project using Cap'n'Proto? Would you recommend providing binaries for the major platforms (Windows, Linux, Mac OS)? I fear a custom C++ build step might add a lot of complexity.

@dwrensha
Copy link
Member

Thanks!

My inclination is to add this as another option in the builder pattern, like this:

fn capnp_executable<P>(&mut self) -> &mut CompilerCommand where P: AsRef<Path> { ...}

Is there any reason that your run_with() would be preferable to that?

@dwrensha
Copy link
Member

Maybe a general question related to the capnp generator: what is considered best practice for a self-contained and portable Rust project using Cap'n'Proto?

One approach might be to run the code generator outside of the main build process and check in the generated code.

Would you recommend providing binaries for the major platforms (Windows, Linux, Mac OS)? I fear a custom C++ build step might add a lot of complexity.

I'm not sure what the best approach would be. I'd be interested to hear any thought you have about how we could better support such things. Note that although "write the capnp tool in Rust" is within the realm of possibility, it would require a large amount of work.

@Bromeon
Copy link
Contributor Author

Bromeon commented May 22, 2020

My inclination is to add this as another option in the builder pattern

That's definitely also an option, and maybe fits the fluent API even more, in the sense that you would always finish the chain with run().

To keep it generically working with &str, &Path etc, I would probably need to store a PathBuf field in CompilerCommand, or do you see another way? Or maybe an Option<PathBuf> and tricky unwrapping to avoid allocation in the default case.

One approach might be to run the code generator outside of the main build process and check in the generated code.

True, but I was thinking of a scenario where you collaborate in a team, and different members have different OSes. It's of course nicer if a project "directly works" 🙂

I'm not sure what the best approach would be. I'd be interested to hear any thought you have about how we could better support such things. Note that although "write the capnp tool in Rust" is within the realm of possibility, it would require a large amount of work.

I assume the generator itself is also (together with the library) part of https://github.com/capnproto/capnproto? Yes, I agree that even if it's a common heard phrase, "rewrite it in Rust" is not necessarily the best choice, especially since you then need to maintain 2 tools (or have the same problem in C++).

For example, Prost, a Rust library for protobuf, ships the precompiled binaries for most common platforms: https://github.com/danburkert/prost/tree/master/prost-build/third-party/protobuf.
If this can be limited to the most common OSes, and the generator tool is stable enough, so that the binaries don't have to be constantly updated, I see it as a viable solution which doesn't require the entire C++ toolchain in place. The disadvantage is of course that there needs to be a way to compile (or cross-compile) on those platforms, and without CI it's quite some work.

@dwrensha
Copy link
Member

To keep it generically working with &str, &Path etc, I would probably need to store a PathBuf field in CompilerCommand.

Yep, that's what I had in mind. We're already storing a bunch of PathBuf objects, so I'm not too worried about the extra allocations.

For example, Prost, a Rust library for protobuf, ships the precompiled binaries for most common platforms: https://github.com/danburkert/prost/tree/master/prost-build/third-party/protobuf.

Interesting!

@Bromeon
Copy link
Contributor Author

Bromeon commented May 22, 2020

Thanks for the again super-fast answer!

It was quite easy to avoid allocation in the default case. But I didn't want to go as far as storing only a unowned reference in CompilerCommand, is this would require the user to mess with lifetimes, which is particularly painful when passing the CompilerCommand around. The process invocation is much more expensive anyway 😉

I also allowed myself to add some assert statements that make sure the one-time methods are not called repeatedly, with the user expectings something different.


Interesting!

Just some thoughts:

On https://capnproto.org/install.html, it looks like you provide binaries for Windows, while you suggest using package managers or building from source on Linux and OS X. One thing to keep in mind is that if binaries are shipped in the official crate, users might expect them to be always up-to-date with latest source. This could add a lot of work on your side or delay releases for only this reason.

An alternative would be a dedicated crate just for the generator tool. Then, the version number would indicate whether the tool is up-to-date or not. Could even have its own Git repository to avoid every user pulling in large binary diffs. Might also simplify setting up things like CI or support for more platforms in the future, if that is ever requested.

It's of course also possible to just start with the most compatible/common platforms (e.g. Win32 + Linux 64bit) and require others to install capnp according to instructions on the download page.

@dwrensha dwrensha merged commit 450d751 into capnproto:master May 22, 2020
@dwrensha
Copy link
Member

Looks good to me. Thanks!

@dwrensha
Copy link
Member

Just some thoughts: ...

Thank you for starting this discussion about distribution of the capnp tool. Please open a separate issue for it if you would like to track it.

@dwrensha
Copy link
Member

I also allowed myself to add some assert statements that make sure the one-time methods are not called repeatedly, with the user expecting something different.

I think the usual way the builder pattern works is that the last call wins. See https://doc.rust-lang.org/std/process/struct.Command.html for example.

Like, imagine I want to provide you with a command that has some default configuration, allowing you to add some more configuration. It would be surprising if your configuration caused a panic -- you would expect it to instead override the existing configuration.

For these reasons, I think I am probably going to remove the assert!() lines.

@Bromeon
Copy link
Contributor Author

Bromeon commented May 23, 2020

Thanks for the merge! Good point about the assert. The only thing to keep in mind is that some of the options can't be "undone" (e.g. no_standard_import()), as such multiple configuration steps cannot always overwrite the previous setting.

I'll create a separate issue for the distribution of capnp.

@Bromeon Bromeon deleted the feature/capnpc-run-with branch May 23, 2020 06:24
ErikMcClure pushed a commit to Fundament-Software/capstone-rs that referenced this pull request Mar 25, 2024
Select custom executable for 'capnp' command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants