Consolidate BUILD.bazel files into a single BUILD file about abseil-cpp HOT 11 CLOSED

One of Bazel's primary goals is to offer "fully hermetic" and reproducible builds. Have you considered the option of referencing Abseil via a WORKSPACE repository-rule, without explicitly cloning the code into your own source tree? This would make it easier to update the version referenced by your team as well.

"""
http_archive(
name = "com_google_absl",
urls = [
"http://github.com/abseil/abseil-cpp/archive/.zip",
],
sha256 = "<ARCHIVE_SHA256_HASH>",
strip_prefix = "abseil-cpp-"
)
"""

where "" is a commit hash from our repository, and "<ARCHIVE_SHA256_HASH>" is the SHA256 of the fully downloaded .zip-file. Note that providing the SHA256 is optional, but will enforce that you are getting byte-for-byte the same archive, every time.

You would then #include our headers like:
#include "absl/strings/string_view.h"

And declare BUILD-dependencies on us, with a syntax like:
deps = ["@com_google_absl//absl/strings"]

Thoughts?

from abseil-cpp.

I strongly suspect this would be a breaking change for existing users - even if we found a way to make things still link properly, it'd be an accumulation of technical debt / disorder. Do you have a proposed migration strategy to get all exsiting users from here to your proposed end state, safely and non-atomically?

from abseil-cpp.

Hi @tituswinters ,

Thanks for the input and the stance that this is being considered.
I totally agree that this would be a breaking change for existing users.
I do have a migration plan that is safe, though atomic. That is actually explained, in part, by the renaming scheme below:

OLD                         NEW

//absl:<config>             :<config>
//absl:<module>             :<module>
//absl:<module>:<library>   :<module>_<library>

For the end users, they need to rename accordingly - and I believe this renaming can be automated.

For example, given a BUILD file like this:

cc_library(
    ...
    deps = [ 
        # Referencing a target that has the same name with the containing directory.
        '@abseil-cpp//absl/strings:strings',
        # Referencing a target that has a different name with the containing directory.
        '@abseil-cpp//absl/strings:strings_test',
     ],
)

It should be changed to

cc_library(
    ...
    deps = [ 
        # Referencing a target that has the same name with the containing directory.
        '@abseil-cpp//:strings',
        # Referencing a target that has a different name with the containing directory.
        '@abseil-cpp//:strings_strings_test',
     ],
)

Note that if we are using abseil-cpp as an external dependency, as specified in WOKRSPACE, this might have seemed like an overkill. But the true value of this change lies in the fact that abseil-cpp can be included in source. With this change, it would be possible to write a BUILD file like this:

cc_library(
    ...
    deps = [ 
        # Referencing a target that has the same name with the containing directory.
        '//third_party/abseil-cpp:strings',
        # Referencing a target that has a different name with the containing directory.
        '//third_party/abseil-cpp:strings_strings_test',
     ],
)

from abseil-cpp.

For the end users, they need to rename accordingly - and I believe this renaming can be automated.

Sure, but how do we do this non-atomically? I guess the constraints on the question aren't clear, lemme expand.

Every .cc file should appear in exactly 1 cc_* target - doing otherwise is an ODR risk.
All of the following have to be true:

• There is a period of time where both new and old target names exist (existing code needs to still build)
• No .cc file can appear in multiple targets (potential ODR / linker failures)
• We produce automation to migrate from the old names to the new names (Abseil refactoring promises this)
• We generally don't delete the old names - there's not a lot of value in doing so, and its annoying.

I think these could be accomplished, but it isn't completely obvious how to do it in the public space. I'm not super interested in working out the details myself, because we've got a quarter of a billion lines of code that already depends on this layout - we're capable of churning that to enact such a change, but I'd rather not if it isn't necessary. And this isn't generally the structure that is used for C++ / bazel projects, so I'm not sold on the value.

You're absolutely welcome to try to convince me, but it will have to include points about the above (linker safety, duplicated .cc's and/or non-atomic refactoring plans, generation of tooling to execute the change). There is some discussion about cutting the absl/strings package into smaller targets - the two ideas may or may not overlap, but they face the same set of constraints (splitting/renaming build targets). So (to my mind) working out the requirements / feasibility on such a refactoring may become a worthy task regardless of exactly the refactoring being proposed.

from abseil-cpp.

Hi @tituswinters ,

I got your points now. Thanks so much for the explanation.

Wow, that is a lot of code depending on this library. Got to automate the transition.

I believe, with some work, all but one constraint can be met:

There is a period of time where both new and old target names exist (existing code needs to still build)

The reason is that if both the existing BUILD.bazel files and the new BUILD file co-exist, they overlap in the .cc and .h files they contain. And bazel will treat that as and error of crossing package boundaries. Yes, I have tried that. It just will not work, even theoretically.

Other constraints you mentioned are relatively easy to do. The one that will require a non-trivial amount of work is a tool that automates the renaming process for projects that already depend on abseil-cpp. I believe that will take a while to produce.

Given all the explanations you have nicely laid out, and my poor man's estimate of the work involved, I no longer believe that my proposed change in this ticket is worth working on.

However, I do still see some value in doing something like the following:

1. Merge my work into the official repo on a branch other than master so that people with similar demands like me have something to refer to. Maybe we can call that branch new-build or something. I would be happy to maintain that branch.
2. If option 1 sounds too much, it would be nice to link to my forked branch in the README or somewhere in the wiki in the official repo and mention that if you want to include absl as a subdirectory in your project, refer to that repo. Again, the idea is to help people with similar needs when they are looking around.

from abseil-cpp.

At this moment the question seems to be whether we want this change in a separate branch, and if yes where we put it. Deferring this to @tituswinters .

from abseil-cpp.

@zhangxy988

Yes. Thanks for ACKing.

from abseil-cpp.

I think I could be more sold on this if you could explain your "needs" in more detail: they seem pretty cosmetic and more like "wants." Is there a deeper technical issue that I'm not seeing that makes this more necessary (rather than just a stylistic choice?)

I completely agree that we need to have more detailed and explicit best-practices for how to operate inter-repo (both for Bazel builds and CMake builds). But that's work that's already ongoing, and that needs a lot of input and design - we need to take that slowly.

from abseil-cpp.

Hi @tituswinters ,

Thanks for the prompt communication. Indeed, I should explain my needs in more details.

So, to begin with, this is not a pull request. I am very well aware that there is probably already a lot of dependencies and ongoing work to make abseil-cpp cater to broader use cases. All I wanted with this issue tracker is to raise the awareness that there are people like me who want to be able to use abseil-cpp in a particular manner.

The fundamental rationale behind all the features I 'want'ed is very simple:

• When I build a software system, I want the source tree to be completely self-contained and the build system fully hermetic.

I know this is not an extremely common use case and people may question its value. Well, I can definitely provide arguments on why I believe there is value in this choice. Without getting into details, I think Google and Facebook deployed this same approach, for a number of reasons. I am trying to employ the same approach of managing source code dependencies as a person instead of as an enterprise.

Admittedly, software developers probably do not care where the dependencies go these days. But how about sysadmins, devops, SREs, Jenkins people? These people really want the source tree to be completely self-contained so that their jobs are easier as they have less dependencies to worry about when building and deploying.

Being fully self-contained means all dependencies are pulled in in source format either directly as a sub-directory or as a submodule. For that purpose, I have Bazelised the build of a few C/C++ projects without modifying the source code:

• openssl
• boost
• ICU4C (up to 60.2 at https://github.com/qzmfranklin/icu)
• glog (recently merged upstream google/glog#232)
• cpython (PR python/cpython#4937, pending review)
• libunwind
• libtorrent
• jemalloc
  ...

One thing I found really useful when writing the BUILD files for these repositories is that the BUILD file should really be written in a way that supports at two use cases:

1. The repository is built as a standalone repository, or is included by other Bazel repos via WORKSPACE.
2. The repository is included in source format as a subdirectory in some other Bazel project, such as the ones I am doing.

Usually it means the usage of native.package_name() properly and handle the $(GENDIR) with or without an external. But that is all it takes.

from abseil-cpp.

This has gone a little stale, but if we decide to do this, Bazel does have target aliases: https://docs.bazel.build/versions/master/be/general.html#alias

I think these create a pretty clear path where the first step moves all of our targets to the top level BUILD file and changes all targets in other BUILD files to deprecated aliases.

That doesn't answer whether this is a change we want to do, though.

from abseil-cpp.

I'm going to close this. It doesn't seem likely that we are going to do this.

from abseil-cpp.