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

First implementation of the Bazel toolchain #8991

Merged
merged 8 commits into from May 30, 2021
Merged

Conversation

tapia
Copy link
Contributor

@tapia tapia commented May 24, 2021

Changelog: Feature: Implements a new experimental conan.tools.google Bazel integration with BazelDeps, BazelToolchain and Bazel.
Docs: conan-io/docs#2109

Close #6235

Shout-out to @technoir42, who worked on a first implementation of the generator (https://github.com/technoir42/conan-bazel)

@tapia tapia marked this pull request as draft May 24, 2021 08:52
Copy link
Contributor

@lasote lasote left a comment

Choose a reason for hiding this comment

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

Looking good and clean!

self._conanfile = conanfile
self._get_bazel_project_configuration()

def configure(self, args=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no configure step for bazel you can nicely remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said to @memsharded in the other PR (I had to close it due to merge complications):

As far as I've seen, the convention when writing the build() method on a project's conanfile.py is:

def build(self):
    bazel = Bazel(self)
    bazel.configure()
    bazel.build(whatever)

Wouldn't deleting the configure method break this pattern?

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily, the MSBuild helper does not have configure() step. It can be removed.

pass

def build(self, args=None, label=None):
# TODO: Change the directory where bazel builds the project (by default, /var/tmp/_bazel_<username> )
Copy link
Contributor

Choose a reason for hiding this comment

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

About the TODO. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's @memsharded's suggestion. He said that the project should be built in the conan's cache. Shouldn't it?

Copy link

Choose a reason for hiding this comment

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

I'm not sure that the concept "the directory where bazel builds the project" makes sense, as much as 'the directory of the Bazel project'

bazel must be run from somewhere in the source tree under a WORKSPACE file. It executes each build action in a symlink tree created for the action and places results in bazel-out in trees under bazel-out and bazel-bin.

After building a particular label or labels, they will appear somewhere under bazel-bin. From there, you might want to copy them out to conan's cache. It's hard to make a particular suggestion without seeing an example of how all the intermediate files should fit together. For example, what would you expect for a configure based project that depends on libfoo where libfoo is built by Bazel.

You obviously need to have the glue that says

  • for target X
  • which produces an output file that you can't easily name (libfoo.a, libfoo.so, libfoo.lib, ...)
  • tell configure how to use that library

Somehow we need to be able to tell configure not to used the installed libfoo. It needs to use the file under bazel-bin. At least, that is what I presume. I don't know the conan workflow enough to know.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @aiuto, thanks for the feedback!

Let me briefly clarify the different flows:

For the consumer point of view: a project using bazel to build that wants to use Conan existing packages as dependencies:

  • This seems to be the most requested use case
  • The BazelDeps Conan generator can output files that Bazel could understand with cc_import, etc instructions to let Bazel locate those Conan packages in the Conan cache. Those packages could have been built with other build system, not a problem, Conan abstracts the package information and can translate it to different build systems.

For the producer point of view, a Conan package which build system is Bazel.

  • This is the case this Bazel Conan helper tries to implement.
  • Conan packages when building from source, are built in the Conan cache, typically in .conan user folder, to not pollute the user projects, and to be able to reuse existing packages among projects.
  • When building from source, Conan also contains a copy of the sources in such ".conan" cache, which might have been retrieved from SCM, for example.
  • For all the other build systems, the build is done in a path that looks like .conan/data/mypkg/1.0/user/channel/build/binary_id/. All the binaries and temporary files are there, and when Conan wants to do the final package, will copy selected artifacts to something like .conan/data/mypkg/1.0/user/channel/package/binary_id/

It would be good if Bazel could be instructed to put the temporary build files in that location, instead of somewhere in bazel-out and bazel-bin, that if I understood correctly, might live in /var/tmp/_bazel by default?

Copy link

Choose a reason for hiding this comment

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

Let's look at the consumer view.
I think a nicely transparant integration is one that points to the conan packages and then creates a local bazel repository that containds a BUILD file with the cc_import rules directly, so that the we can refer to them directly.
So the user experience is this:

WORKSPACE

workspace(name = 'myproject')
conan_repository(
  name = "foo", 
   artifacts=['a, 'b', .. list of top build targets from the conan package...]
)

BUILD

cc_library(
  name = 'myapp',
  deps = ['@foo//:a']
  ...
)

and the glue generated by the conan_repository rule is

WORKSPACE

workspace(name=foo)

BUILD

cc_import(
  name = 'a',
  hdrs = [exported headers of the artifact a],
  static_library = "libmylib.a",
)

libmylib.a: symlink to libmylib.a built as a result of doing the conan build.

Copy link
Contributor Author

@tapia tapia May 31, 2021

Choose a reason for hiding this comment

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

The merged generator is pretty similar to your idea. The dependencies are declared in the conanfile.py, though, not in the WORKSPACE file. For example:

class BazelExampleConan(ConanFile):
    name = "bazel-example"
    ....
    build_requires = "boost/1.76.0"

Then, the only thing you have to add to the WORKSPACE file is this:

load("@//conandeps:dependencies.bzl", "load_conan_dependencies")
load_conan_dependencies()

After that, just update the BUILD files where you need to use the new dependency:

cc_binary(
    name = "hello-world",
    srcs = ["hello-world.cc"],
    deps = [
        "@boost//:boost",
    ],
)

Give it a try and let us know what you think :-)


@pytest.mark.skipif(platform.system() != "Windows", reason="Only for windows")
class WinTest(Base):
@parameterized.expand(["Debug"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Not very "expandable" right? LoL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's obviously temporal. The bazel generator can't handle build types yet, but it will 😄

@tapia tapia marked this pull request as ready for review May 26, 2021 07:49
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.

Looks good to me for a start and getting users feedback. Lets add it to 1.37?

self._conanfile = conanfile
self._get_bazel_project_configuration()

def configure(self, args=None):
Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily, the MSBuild helper does not have configure() step. It can be removed.

@memsharded memsharded added this to the 1.37 milestone May 27, 2021
@memsharded memsharded requested a review from lasote May 27, 2021 12:52
@memsharded memsharded mentioned this pull request May 27, 2021
1 task
@memsharded
Copy link
Member

Let's merge this and released it in 1.37 (tomorrow), even if there is feedback to check, and potential follow-up improvements, this has 0 risk, and better have it merged for further feedback and testing in 1.37 release.

@memsharded memsharded merged commit a5676f4 into conan-io:develop May 30, 2021
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.

[feature] Generator for Bazel
4 participants