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

Feature: Enable bazel >= 7.1 #16196

Open
wants to merge 7 commits into
base: develop2
Choose a base branch
from

Conversation

Neeeflix
Copy link
Contributor

@Neeeflix Neeeflix commented May 3, 2024

Feature: Add support for Bazel >= 7.1. This is also requested in #15754.

This will not break older bazel versions, as just some additional files are generated.

I added some of those file based test-cases, and did some "real" integration tests locally. If you know any better better way to put this into the conan test-environment, please let me know.

It's "only" supporting versions from 7.1 on, as there is a feature being used which is not available in 7.0. This is also documented in the code.

Docs: conan-io/docs#3707
Example: conan-io/examples2#147

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@Neeeflix
Copy link
Contributor Author

Neeeflix commented May 3, 2024

@memsharded thanks for adding the milestone and looking into this.
Would it also be possible to add a milestone for 1.X, as for us it will not be easy to go with 2.X in the near future.

Any work that needs to be done here, could be taken over by me. I think there are some smaller differences between the 1.X generator and the 2.X one

@memsharded
Copy link
Member

Would it also be possible to add a milestone for 1.X, as for us it will not be easy to go with 2.X in the near future.

This is not something that we can keep doing much longer, for new features like this one we'd strongly favor Conan 2 only. It was released more than 14 months ago, ConanCenter will soon stop updating packages for Conan 1.X too. We strongly recommend to prioritize as much as possible the update to Conan 2.

In this case, if you'd like to get this in Conan 1, and you are willing to do the effort, then we might want to wait until we get @franramirez688 feedback about this.
One of the aspects is that we should be 100% sure that it is not breaking Conan 1.X users, if there is any minimal risk (and I am a bit concerned about the proposed changes could no work for users using older versions of Bazel). @franramirez688 is more aware of the possible issues and efforts related to this.

@Neeeflix
Copy link
Contributor Author

Neeeflix commented May 3, 2024

This is not something that we can keep doing much longer, for new features like this one we'd strongly favor Conan 2 only. It was released more than 14 months ago, ConanCenter will soon stop updating packages for Conan 1.X too. We strongly recommend to prioritize as much as possible the update to Conan 2.

I understand the urgency and importance of updating to Conan 2, especially with the upcoming discontinuation of updates for Conan 1.X. However, the process presents significant challenges, especially when depending on teams in different organizations, etc.

One of the aspects is that we should be 100% sure that it is not breaking Conan 1.X users,

As mentioned in the PR description, the files which are generated for the older bazel version are not touched.
There are only two additional files being generated, there is no interference between those files at all.

@Neeeflix Neeeflix force-pushed the feature/enable-bazel-7.1 branch 2 times, most recently from 605d489 to c0d8468 Compare May 7, 2024 10:47
@franramirez688
Copy link
Contributor

Hi @Neeeflix

Thanks a lot for this PR! 👏
I was trying to test it locally but I got stuck with this error:

INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.

hello/0.1: RUN: bazel --bazelrc=/Users/franchuti/.conan2/p/b/hellodfdcbdac82faa/b/conan/conan_bzl.rc build --config=conan-config //main:hello
WARNING: --enable_bzlmod is set, but no MODULE.bazel file was found at the workspace root. Bazel will create an empty MODULE.bazel file. Please consider migrating your external dependencies from WORKSPACE to MODULE.bazel. For more details, please refer to https://github.com/bazelbuild/bazel/issues/18958.
ERROR: Error computing the main repository mapping: in module dependency chain <root> -> bazel_tools@_ -> rules_python@0.22.1: Error accessing registry https://bcr.bazel.build/: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
Computing main repo mapping:

hello/0.1: ERROR:
Package '015c501fa3ba2da21e4c2e71becb7b76fa605a36' build failed
hello/0.1: WARN: Build folder /Users/franchuti/.conan2/p/b/hellodfdcbdac82faa/b

I do not know how to solve this. I just tried a couple of things that I found here:

Nothing worked 😭 Do you know how I can get it running OK locally?

@Neeeflix
Copy link
Contributor Author

Neeeflix commented May 8, 2024

Hey @franramirez688,
Thanks for looking into this

Regarding the your issue:

INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.

hello/0.1: RUN: bazel --bazelrc=/Users/franchuti/.conan2/p/b/hellodfdcbdac82faa/b/conan/conan_bzl.rc build --config=conan-config //main:hello
WARNING: --enable_bzlmod is set, but no MODULE.bazel file was found at the workspace root. Bazel will create an empty MODULE.bazel file. Please consider migrating your external dependencies from WORKSPACE to MODULE.bazel. For more details, please refer to https://github.com/bazelbuild/bazel/issues/18958.
ERROR: Error computing the main repository mapping: in module dependency chain <root> -> bazel_tools@_ -> rules_python@0.22.1: Error accessing registry https://bcr.bazel.build/: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
Computing main repo mapping:

hello/0.1: ERROR:
Package '015c501fa3ba2da21e4c2e71becb7b76fa605a36' build failed
hello/0.1: WARN: Build folder /Users/franchuti/.conan2/p/b/hellodfdcbdac82faa/b

Can you may give some more details about your setup. Which example did you use?
I also updated the example in: conan-io/examples2#147, can you try that?
Also noticed that I only tested there for conan 1.X. Updated it, now also conan 2.X works for me.

Reproduction steps:

  1. Install conan from this branch
  2. CD to the example repo and checkout my branch.
  3. In examples/tools/google/bazel_7.1/testsuite I run conan build . -of conan
  4. image

@franramirez688
Copy link
Contributor

Hi @Neeeflix

Thanks a lot for asking and for your help. Finally, after several hours of trying crazy things, I found the solution to my problem. I'm using Zscaler, so the solution came up with importing through keytool the Zscaler root certificate 👏
Now, I can start reviewing everything 🥳 Sorry for the long delay in keeping my environment up-to-date.

Copy link
Contributor

@franramirez688 franramirez688 left a comment

Choose a reason for hiding this comment

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

@Neeeflix

Finally, I was able to try this. Thanks again for the PR to support Bazel >= 7.1 versions and the huge work behind this. It looks promising!

It's working locally in macOS and Win 👏 , but I'm having some problems with the certificates & Java again in Linux (I hope I get it ready ASAP).

Meanwhile, let me know more about this approach. Sorry for my dummy questions but I want to understand everything 😁

"//build:conan_deps_module_extension.bzl",
"load_dependencies"
)
use_repo(load_conan_dependencies, "dep-1", "dep-2", ...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Then, the UX is something like this:

load_conan_dependencies = use_extension("//conan:conan_deps_module_extension.bzl", "load_dependencies")
use_repo(load_conan_dependencies, "dep-1", "dep-2", ...)

So, is it not possible to make something generic as it was done before?

load_conan_dependencies()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, with this approach this will not be possible.

But when I compare this to Cmake, there you also have duplicated information like this regularly.
I would say it could be compared to a find_package call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree with that 👍

for child in child_packages:
rctx.symlink(child, child.basename)

rctx.symlink(rctx.attr.build_file_path, "BUILD.bazel")
Copy link
Contributor

Choose a reason for hiding this comment

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

Symlinks always scare me, but it looks like there is no other way to do this.
I need to see how it'd work in more complex scenarios, e.g., using the same dependency as a tool_require and a normal requirement (defining the build_context_activated).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Symlinks always scare me, but it looks like there is no other way to do this.

I mean there is an alternative, which would be to copy the files. But I'm not sure if this would really be better.
The symlinks point to files in the package folders of Conan packages, those should not be touched under normal conditions, or?
I'm also pretty sure, that the legacy method does the same thing.

I need to see how it'd work in more complex scenarios, e.g., using the same dependency as a tool_require and a normal requirement (defining the build_context_activated).

Yeah, this is quite interesting. I did not really look into this aspect. But this is anyway the same situation for the legacy Bazel tooling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks a lot for the explanation. Yeah, I think this is a more efficient approach, so let's give it a try with those complex scenarios.

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.

None yet

4 participants