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

initial setup of bazel toolchain for qt #28

Merged
merged 8 commits into from
May 15, 2021
Merged

initial setup of bazel toolchain for qt #28

merged 8 commits into from
May 15, 2021

Conversation

justbuchanan
Copy link
Owner

The goal is to separate rule implementations from the platform-specific
binaries and settings needed. To start, the toolchain provides
hard-coded paths to the moc, rcc, and uic tools. tools/BUILD provides
separate values for Windows and Linux.

The next step, I think, is to use qt_configure.bzl to dynamically
determine the paths to these tools on the current system.

Bazel docs on toolchains: https://docs.bazel.build/versions/master/toolchains.html.

The goal is to separate rule implementations from the platform-specific
binaries and settings needed. To start, the toolchain provides
hard-coded paths to the moc, rcc, and uic tools. tools/BUILD provides
separate values for Windows and Linux.

The next step, I think, is to use qt_configure.bzl to dynamically
determine the paths to these tools on the current system.
@justbuchanan
Copy link
Owner Author

@limdor, let me know if you have any thoughts on this, I know you've spent some time looking into bazel toolchains. I tried locally merging these changes into #23 and with some minor changes to _gen_ui_header() to use the toolchain-provided binary, I was able to get it to build on windows and linux.

Copy link
Contributor

@limdor limdor left a comment

Choose a reason for hiding this comment

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

@justbuchanan I just added some comments based on some of the thinking that I did on toolchains. I added some links that can also help you to see what ideas I had in mind.
In addition I created a draft PR with what I had in progress, it was not much but was just to show the approach that I was taking and in the end is very similar to what you are doing here.
#29

I would be interested on knowing how you solved locally the problem with the ui rule, the problem is that ui is quite picky on the path inside the ui files and how this relates to the working directory and to the ui location.

Also some of the initials ideas I was discussing them with @aiuto in this thread:
https://groups.google.com/g/bazel-dev/c/Y8LOU-CxZkw/m/4zlmpGUKBQAJ

@@ -11,3 +12,5 @@ new_local_repository(
build_file = "@com_justbuchanan_rules_qt//:qt.BUILD",
path = local_qt_path(),
)

register_qt_toolchains()
Copy link
Contributor

Choose a reason for hiding this comment

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

My idea regarding qt_configure and register_qt_toolchains was that in the future both are merged in one single call and you only need to call register_qt_toolchain. One option to do that is with template files, similar to what is done in rules_go https://github.com/bazelbuild/rules_go/blob/4cd45a2ac59bd00ba54d23ebbdb7e5e2aed69007/go/private/sdk.bzl

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sounds good, I agree it would be best to put everything in just a single function that needs to be called from WORKSPACE. I'll look at refactoring this to do so.

@justbuchanan
Copy link
Owner Author

Thanks for reviewing this and for the draft PRs. After I merge this, I'll take a look and see what else I can merge in from #29. I'll also send you my change to get #23 to build.

@justbuchanan justbuchanan merged commit c6ecbab into master May 15, 2021
@justbuchanan justbuchanan deleted the toolchain branch May 15, 2021 15:44
@justbuchanan
Copy link
Owner Author

Here's the change to gen_ui_header for using the toolchain: 8648c74.

@limdor
Copy link
Contributor

limdor commented May 15, 2021

Here's the change to gen_ui_header for using the toolchain: 8648c74.

Interesting that with this works, not sure if the working directory then with toolchains is different. Anyway, cool that works!

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