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

[Bazel][1.x][Part 1]: Bazel + BazelToolchain refactor #14958

Merged
merged 24 commits into from
Oct 30, 2023

Conversation

franramirez688
Copy link
Contributor

@franramirez688 franramirez688 commented Oct 18, 2023

Changelog: Feature: BazelToolchain creates a conan_bzl.rc file which defines the conan-config configuration. If it exists, Bazel helper will use it automatically.
Docs: conan-io/docs#3457
Closes: #14282 #14519

Next PR (BazelDeps): #14959

Overall, this PR is introducing no compatible changes, but deprecating some existing variables.

Summary:

  • BazelToolchain is generating a conan_bzl.rc file with a build:conan-config configuration depending on your settings/options.
  • BazelToolchain is not generating a real Bazel toolchain (https://bazel.build/extending/toolchains) because they require much more knowledge and expertise in Bazel to make a dynamic Bazel toolchain per user configuration.
  • Bazel is moving forward to platforms (https://bazel.build/extending/platforms) for future releases instead of toolchains but it does not imply that BazelToolchain could be incompatible but some changes to inject those platforms constraints in our conan-config configuration.

@franramirez688
Copy link
Contributor Author

franramirez688 commented Oct 19, 2023

@czoido seems that bazel is not installed in macOS machines 😢

The same test was already passing correctly for Conan 2.x (#14845)

UPDATED:
@czoido Tests passing, solved! 👏

conan/tools/google/bazel.py Outdated Show resolved Hide resolved
@czoido
Copy link
Contributor

czoido commented Oct 26, 2023

I think there's a problem that we should resolve on how Bazel caches the artifacts and Conan packages it after the build.
The problem is that if for example, you create a bazel package and the library is packaged ok, because the artifacts are taken from the Bazel cache to the Conan cache, but then you make some modifications to the package, and change the name of the library for another one, then the old and new library are taken from the Bazel cache and two libs are packaged. To reproduce:

conan new mypackage/1.0 -m bazel_lib
conan create . -> libmypackage.a is stored in Conan cache
# replace name = "mypackage", in the BUILD file for name = "mypackagewithothername",
conan create . -> libmypackage.a and libmypackagewithothername.a are both stored in Conan cache

raise ConanException("Windows and Macos needs extra BUILD configuration to be able "
"to create a shared library. Please, check this reference to "
"know more about it: https://bazel.build/reference/be/c-cpp")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The next PR solves this issue. That one introduces changes in the template to let users create/consume shared libraries in all the platforms.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Overall looking good, but my concerns about breaking too much, and I'd try to minimize it.

conan/tools/google/bazel.py Show resolved Hide resolved
conan/tools/google/layout.py Show resolved Hide resolved
conan/tools/google/bazel.py Outdated Show resolved Hide resolved
conan/tools/google/bazel.py Outdated Show resolved Hide resolved
conan/tools/google/layout.py Outdated Show resolved Hide resolved
conans/test/functional/toolchains/google/test_bazel.py Outdated Show resolved Hide resolved
conan/tools/google/bazel.py Outdated Show resolved Hide resolved
Comment on lines +11 to +12
conanfile.output.warning("In bazel_layout() call, generators folder changes its default value "
"from './' to './conan/' in Conan 2.x")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can make here a explicit recomendation of setting self.folders.generators_folder = "conan" so that users can have a clue on what to do, but we can change that in the next PR

@czoido czoido merged commit 49bc8e5 into conan-io:develop Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants