capnpc: add option to build capnp exe from src
#268
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I like this approach. |
Bromeon
reviewed
May 24, 2022
Comment on lines
+71
to
+74
| let mut p = PathBuf::from(env!("OUT_DIR")); | ||
| p.push("bin"); | ||
| p.push("capnp"); | ||
| p |
There was a problem hiding this comment.
Probably a matter of style, but you could just use:
PathBuf::from(env!("OUT_DIR"))
.join("bin")
.join("capnp")
Comment on lines
+44
to
+46
| [build-dependencies] | ||
| cmake = "0.1" | ||
| which = "4.2" |
There was a problem hiding this comment.
cmake should probably be marked optional, while activated by build-capnp feature?
[features]
build-capnp = ["dep:cmake"]
[build-dependencies]
cmake = { version = "0.1", optional = true }
which = "4.2"See also Cargo reference. The dep: syntax requires Rust 1.60.
Comment on lines
+8
to
+13
| if !which::which("capnp").is_ok() { | ||
| panic!( | ||
| "capnp executable not found. install it with your package manager or enable the \ | ||
| \"build-capnp\" feature to build it from source" | ||
| ); | ||
| } |
There was a problem hiding this comment.
if !which::which("capnp").is_ok() {
panic!(msg);
}could be written as:
which::which("capnp").expect(msg);
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #182.
This is one potential way we could implement option
2.)while keeping in mind @zenhack's concern about pushing users away from their package manager. The idea is that we check for the existence ofcapnpat build time. If it does not exist, we throw an error letting the user know to either install it OR providefeature = "build-capnp"to the crate so that we can build it for them.I think that it is good to at least provide this option to the user since many may find it desirable to have a plug-and-play solution without the need for prerequisites when building their crate.
You may find adding the
capnprotosubmodule undesirable, in which case we could potentially clone the repo at build time.