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

Add Bazel integration #1255

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add Bazel integration #1255

wants to merge 9 commits into from

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Feb 21, 2023

Fixes #478

This patch adds Bazel Integration.

It can build:

  • autocxx-* libs/binaries - please see BUILD in the top directory.
  • demo - please see demo/BUILD.

,which follows the structure of cxx project. So CI also built the demo app.

The commits contain a tons of file change but most of them are autogen files.
The autogen and non-autogen files are separated so please refer to the commit message.

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to README are included in PR

@nak3
Copy link
Contributor Author

nak3 commented Feb 21, 2023

Though I opened this PR I know I used some "hacky" way in demo/BUILD
so I don't mind if this PR was refused or keep it open until the satisfied solution is found.

BUILD Outdated
"//third-party:proc-macro2",
"//third-party:quote",
"//third-party:serde",
"//third-party:thiserror",
Copy link
Collaborator

@adetaylor adetaylor Feb 21, 2023

Choose a reason for hiding this comment

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

Thanks for this PR.

However, I hadn't realized that bazel support would require hard-coding all these dependencies. That's not OK with me - the Cargo.toml files need to be the sole source of truth - can you instead add a script or tool to generate these BUILD files based on the Cargo.toml?

More generally, I'm concerned that the project structure is not static enough to maintain a duplicate set of build instructions. It feels like it would be appropriate to check in such BUILD files (or equivalent) only when/if autocxx reaches 1.0 and we're therefore relatively confident that big structural changes will be unusual. I don't think we're at that point yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. Yes, it was possible to drop the hard-coding deps in BUILD by all_crate_deps. I missed this utility.

WORKSPACE Outdated
rules_rust_dependencies()

rust_register_toolchains(
versions = ["1.66.0"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this dependent on the CI environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it is not necessary. It should be alright for stable and nigthly which will be registered as per http://bazelbuild.github.io/rules_rust/.

@@ -0,0 +1,42 @@
[workspace]
[package]
name = "third-party"
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are generated files, right? Please check in the tool/procedure to generate them, instead of these files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I could drop these files. Now the generated files are only under third-party/bazel/* generated by bazel run //third-party:vendor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the lack of comment here - I've been travelling a bit lately.

I'm still not OK with these being checked in. I want to be able to upgrade dependencies by changing only Cargo.toml files - then everything else needs to be autogenerated.

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.

Bazel Integration
2 participants