-
Notifications
You must be signed in to change notification settings - Fork 154
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
Generate fixed filenames for all types of files #876
Conversation
Also update the documentation about how running `gen` with fixed filenames works. Fixes google#773
Given that the coverage of these tests still isn't too great, I also tested with my full use case before opening this PR. I think everything still compiles, but there's some unrelated new issues to chase down first. |
I can't get my local clippy to show these for some reason...
demo/src/main.rs
Outdated
@@ -21,6 +21,9 @@ include_cpp! { | |||
generate!("Goat") | |||
} | |||
|
|||
#[link(name = "autocxx-demo")] | |||
extern "C" {} | |||
|
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.
Why this change?
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.
That's how the .rs files generated by integration-tests get the static library linked in, and it seemed like a reasonable thing to add for a complete demo that can be built either way the docs mention.
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.
Ah right, so this is on the basis that the demo code should build using autocxx-gen
as well as autocxx-build
?
I'd prefer to keep the demo code as simple as possible for the Cargo (autocxx-build
) world, where this is already taken care of by the build.rs
. I think anyone deviating from Cargo knows that they will need to jump through some hoops, and if they're using blaze/bazel/gn or similar, this link
directive probably won't do the trick anyway.
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.
Yes, exactly.
In that case, any thoughts on adding something for autocxx-gen
tests to build? Could copy the whole directory, but that seems annoying.
Also, I'll back the tests out of this PR for now in the interests of getting the functional change merged.
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.
Ah right, it's used by the test code, that's the bit I hadn't realized.
Yeah - I'd still strongly prefer to avoid adding these lines into the demo. I want that to be focused on the simplest possible experience for cargo users. I'd be OK with adding another copy of equivalent files for the autocxx_gen
tests, or programmatically writing files out from the test code.
This looks good - sorry I didn't get to it till now. Just a few nits. |
Looks like this needs more discussion, splitting it out to avoid distracting from the functional changes in this PR.
Split out from google#876.
Also update the documentation about how running
gen
with fixedfilenames works.
Fixes #773