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

feat: adding VulkanSceneGraph v1.1.7 library to BCR #3118

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

Conversation

artem-korolev
Copy link

Hi,

I am quite new to Bazel BCR. Want to add VSG as a module. Already build it successfully locally with BUILD file provided in this PR.
It probably missing all scenarios that VSG support in their Cmake project. And I tested it with Linux system (Ubuntu 24.04) only.
But our team actively working on a game engine, so all platforms will be supported in feature (macos, linux, windows, android). So for now I put there a version "1.1.7.bcr.1", meaning that it is a first draft version, that I want to test out.

Copy link

google-cla bot commented Nov 5, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@bazel-io
Copy link
Member

bazel-io commented Nov 5, 2024

Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (vsg) have been updated in this PR. Please review the changes.

@meteorcloudy
Copy link
Member

Can you make the version 1.1.7-beta1 or similar names? 1.1.7.bcr.1 usually means there are registry files only change and often implies there is already a 1.1.7 previously.

@meteorcloudy
Copy link
Member

@bazel-io skip_check unstable_url

@meteorcloudy meteorcloudy added the skip-url-stability-check Skip the URL stability check for the PR label Nov 6, 2024
Vertexwahn
Vertexwahn previously approved these changes Nov 6, 2024
@Wyverald Wyverald added the presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval label Nov 6, 2024
@bazel-io bazel-io dismissed Vertexwahn’s stale review November 6, 2024 18:29

Require module maintainers' approval for newly pushed changes.

@artem-korolev
Copy link
Author

artem-korolev commented Nov 6, 2024

Dear BCR team, thank you for your help! Now I need to add one more module - vulkan 1.1 headers - this is the only required dependency for VSG 1.1.7. locally I have it installed globally, but on build agents its of course not installed.
Will do this in separate branch. will come back here shortly, when vulkan 1.1 will be accepted.

need to setup hermetic local environment at some point to have better experience working on BCR modules :)

@Vertexwahn
Copy link
Contributor

@artem-korolev There were already some WORKSPACE approaches for Vulkan e.g. https://github.com/zaucy/rules_vulkan - since you need also the SDK

@artem-korolev
Copy link
Author

artem-korolev commented Nov 6, 2024

@Vertexwahn thank you. good point. Will try to apply it. Also found a platform specific dependency. On Linux it is XCB (#include <xcb/xcb.h>). Need to find out how to deal with that as well

@bazel-io
Copy link
Member

Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (vsg) have been updated in this PR. Please review the changes.

@artem-korolev
Copy link
Author

Hi. Im a bit stuck here. Im trying to integrate rules_vulkan. But it does not build on build machines. I limited it to build only on ubuntu 24.04, to have more/less system like I have.

I think i got here into some conceptual problem. I was trying to do everything with MODULE.bazel (extending it with *.bzl extensions), but it did not fly (i do something wrong and I cannot find example solving same issue).
So i then did it locally using WORKSPACE. It builds locally and it finds "@vulkan_sdk//:vulkan" dependency just fine. Published it here - https://github.com/artem-korolev/vsg_bzlmod_build_test

So not I understand that problem on build machine is that it does not support WORKSPACE files. I read docs, that WORKSPACE will be deprecated in bazel 8.x. But it still works in 7.x (which I can confirm building in locally). And found this issue - bazelbuild/bazel#18958, where its said, that bazel will fallback to use WORKSPACE. but maybe I misread something.

So my question is, can I still use WORKSPACE? If not, then what will be easier and more convenient: refactor rules_vulkan, or integrate it to MODULE.bazel with newer api? In workspace its using http_archive (which i found how to use in MODULE.bazel), but then it runs some python functions (setup_7zip and vulkan_repo) - this I already do not understand how to reproduce in MODULE.bazel

And problem is that on build machine it does not find "vulkan_sdk" repo. So repo is not registered.

@Vertexwahn
Copy link
Contributor

WORKSPACE files are dead. Bzlmod for the win! We have to come up with a Bzlmod variant of rules_vulkan. I am currently busy with trying to get Qt working with Bzlmod -> https://github.com/Vertexwahn/rules_qt6/tree/bzlmod2 - maybe you can copy some ideas - it also fetches different Qt versions for different OS versions - rules_vulkan has a dependency to rules_7zip - e.g. first rules_7zip need to be migrated to Bzlmod. Another approach could be using an existing Vullkan installation - e.g. https://github.com/jadarve/rules_vulkan. I suggest to first to migrate rules_7zip to Bzlmod and then rules_vulkan.

@artem-korolev
Copy link
Author

Hi @Vertexwahn, thank you for such a quick answer! I got my answer I needed and ideas. Can continue experimenting now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval skip-url-stability-check Skip the URL stability check for the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants