-
Notifications
You must be signed in to change notification settings - Fork 213
Add a working introduction to Bazel. #17
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
Add a working introduction to Bazel. #17
Conversation
working/bazel/module-structure.md
Outdated
``` | ||
|
||
Here we have _3_ targets: | ||
* `app`, which potentially is the entrypoint to our package/application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit concerned that this wording further promotes putting application entrypoints under lib/
, which isn't recommended and makes tooling more difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified a bit, thanks
In this case, `package:ninjacat.common.widgets.login.testing` exposes a set of | ||
test-only libraries that can be used, only in _test_ targets. This pattern isn't | ||
replicable externally at all, but is _very_ common internally to avoid bringing | ||
in test utilities and code into production applications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting insight - I could imagine a dev_only
flag which could be set which only allows a package to be used as a dev dependency or as a direct dependency of other dev_only
packages - this would be non-breaking for pub (but a breaking change per package when they opt in).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
working/bazel/module-structure.md
Outdated
@@ -0,0 +1,226 @@ | |||
The following is a reference for [`Bazel`](https://bazel.build/) (internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] "Internally"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
working/bazel/module-structure.md
Outdated
We map the concept of `pub` packages with the following rules: | ||
|
||
* A `package:<a>.<b>/uri.dart` resolves to `//a/b/lib/uri.dart`. | ||
* Any other `package:<name>/uri.dart` resolves to `//third_party/dart/<name>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true internally, but not in dart-lang/rules_dart (Where it would be @<name>/lib/uri.dart
IIRC)- I know we already called out our lack of bazel support so maybe this doesn't matter much here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
working/bazel/module-structure.md
Outdated
> BUILD | ||
``` | ||
|
||
(If the package happens to be a Dart web _entrypoint_, you might also see `web/`.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might also see bin/
for VM binaries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
## Notable Differences | ||
|
||
### No cyclical _targets_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth mentioning that this is actually quite common when you take into account dev dependencies. If you have a testing only package (angular_test, build_test), which depends on the main package but is also used within the main packages tests (very common) now you have a cyclic dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
/cc @leafpetersen @lrhn @munificent Is this useful enough to check-in as-is? Is there anything else you wish you knew (or could reference) that could be added on a follow-on? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some random comments on pub that I'm not sure are useful or not.
I'll let Leaf and Lasse comment on whether this should be landed. Either way, it was really useful to me to read.
working/bazel/module-structure.md
Outdated
* `flags_test`, which tests that `flags` is working-as-intended. | ||
|
||
This concept already is very different than a `pub` package, where all of the | ||
files in `lib/` are sort of a single "implicit" target. In fact, a common issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on what you mean by "target" here, but it's always been expected and allowed to have multiple libraries inside "lib" that are each their own separate thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clarified the wording a bit to explain what I actually meant :)
working/bazel/module-structure.md
Outdated
files in `lib/` are sort of a single "implicit" target. In fact, a common issue | ||
externally is that `pubspec.yaml` (sort of similar to `BUILD`) is not granular | ||
enough, leading to the creation of "micro packages" that have a single file or | ||
capability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to think of those micro packages as an anti-pattern. :( They can be useful if you really need to have different subsets of dependencies, but otherwise they just make maintenance and consumption of those packages harder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most common cases, so far, which we haven't been able to help with:
{name}_test
packages (see below, basically avoiding a dependency onpackage:test
)- X-plat packages (i.e. a different implementation for web, I/O, flutter) might want different deps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the concerns here, but my impression is that fears around dependencies are somewhat overblown. Just depend on the union of the packages that the various platforms need. In almost all cases, the version solver should be able to handle them.
I know that hasn't always worked out well in practice, largely because the analyzer went through a period of very enthusiastic breaking version releases and many things transitively depend on it. But that's settled down and things should work pretty reliably now. Also, @nex3's new version solver is much better at handling these cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we ever get to a period that all core packages rarely change or break, I can agree with that. There are other issues too, like modular/incremental builds that are hampered by this (or at the very least made difficult to compute).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that a build system of some sort is essentially required for all Dart implementations, we should be a lot more careful about huge sets of extra dependencies.
The entire package graph and all the sources of all packages has to be considered during the build, and this can cause considerable bloat for things like DDC/Kernel Builders which operate on all files in all packages. Even if they never actually run because those files aren't imported, we still have to set up the asset graph such that they could run on anything, and some initialization and finalizations steps do have to iterate all potential nodes in the graph.
working/bazel/module-structure.md
Outdated
### No cyclical _targets_ | ||
|
||
Dart, as the language, allows cyclical dependencies between `.dart` files | ||
libraries). Bazel does _not_ allow cyclical dependencies between packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "(" before "libraries".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
) | ||
``` | ||
|
||
Unfortunately this is quite common when you take into account the concept of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only unfortunate if you want to treat an entire pub package as a single unit of compilation/modularity, which was never the goal.
A pub package's granularity is the unit of "shippable API product". It's a version manager, not a build tool. Finding a pub package lacking as the unit of compilation is like complaining that you can't recline an entire car. That's not the point. It's a container around things that can be reclined. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you can't say micro packages are an anti-pattern and say pubspec.yaml
is only about version solving :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what we probably want are three levels of granularity:
-
Shippable versioned API product. These have names, release schedules, dependencies on other products. Cycles are allowed. Pub packages.
-
Build target. This is the unit of compilation and modularity. Likely the unit of encapsulation for things like "sealed". Possibly having a privacy control at this level. Acyclic. This doesn't currently exist in a user-facing way.
-
Source file. A single file of source code with its own top-level scope. Privacy and namespacing works at this level. Dart libraries. Cycles are allowed (and heavily used in practice).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The levels of granularity you describe @munificent are exactly what we have in bazel, with the only difference being that packages are defined as directories with BUILD files, instead of coming from pub. But conceptually they are the same.
What @matanlurey and probably most people who have tried to integrate with a Dart compiler wants/needs, is a formal and consistent way of defining the Build target portion externally. I believe this should be integrated with pub, for a couple reasons:
- You need to have separate dependencies per build target in order to efficiently create an acyclic graph of build targets.
- The only alternatives I can come up with are constantly re-analyzing your actual Dart import structure to create custom targets (what package:build does, at significant expense), or forcing all end users to create static configuration for all their transitive dependencies, which is just not realistic.
- Allows the build target structure to be statically configured, which is a huge win for build systems. Neither bazel nor package:build_runner allow you to read files before declaring your inputs/outputs, so this is a huge pain point today.
- Module granularity/structure is consistent across tools.
- No extra dependencies for platforms/features you don't use.
- Bazel has proven that configuring build targets in the package itself works well (enough). If you need a more granular target you can always send a PR to a package to create it. See my next point for where this potentially falls down externally though...
All this being said there is one potentially huge issue with trying to push the bazel model on the external world, which is figuring out how to actually enforce the acyclic requirement of the build targets. With bazel and a single version restriction for packages, this is not really an issue. But for pub packages any new non-breaking release of a package could introduce a build target cycle for any package that depends on that package accidentally. Maybe the version solver could take that into account (throw out versions if they introduce a build target cycle?), but it doesn't seem trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. (We should probably add a doc about build system constraints somewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all I find the document informative, but a little vague on its goals and scope. It describes the Google Blaze build system and repository structure in detail, but doesn't say why this is interesting to anyone outside of Google.
@@ -0,0 +1,234 @@ | |||
The following is a reference for [`Bazel`](https://bazel.build/) (internally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internally -> internally in Google
(This repository is not aimed at Googlers only).
Consider whether it's important to external readers at all, it probably isn't.
Much of the document seems to be about Google-only design choices, so maybe instead make it explicit that this document is mainly about Blaze at Google, but that it also applies to Bazel build systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
(Bazel is currently Google only for the most part, but that is rapidly changing. For example SpaceX uses it as their own build system, and many other medium/larger companies are considering it)
This is *not* a proposal, nor a commitment to build out further Bazel support | ||
externally, but rather a reference to help guide discussions around visibility | ||
modifiers and terminology around modular/reusable Dart code. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is this document intended for?
That is, who would care about Bazel modules at all? And why should they? (So, I guess I'm asking for motivation - to make Bazel compatibility visible in language discussions - and expected audience to be spelled out).
I'm guess it is for people like me, who has no working knowledge of Bazel or Blaze, but who work with Dart pub and language packages all day long, and who might affect how well future language changes matches the Bazel model.
of any language (including Dart) to change these requirements. | ||
|
||
[1]: https://cacm.acm.org/magazines/2016/7/204032-why-google-stores-billions-of-lines-of-code-in-a-single-repository/fulltext | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that Bazel use is a Google-only requirement that nobody else should worry about? (Again, who is the target audience?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Clarified.
working/bazel/module-structure.md
Outdated
We map the concept of `pub` packages with the following rules: | ||
|
||
* A `package:<a>.<b>/uri.dart` resolves to `//a/b/lib/uri.dart`. | ||
* Any other `package:<name>/uri.dart` resolves to `//third_party/dart/<name>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does "any other" here mean a package with no .
in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Clarified.
working/bazel/module-structure.md
Outdated
|
||
### Inside a Package | ||
|
||
Once _inside_ a package, such as `//ninjacat/app`, you can expect the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "Once". It confuses me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* `flags`, which contains some common code for setting/getting flags. | ||
* `flags_test`, which tests that `flags` is working-as-intended. | ||
|
||
This concept already is quite different than a `pub` package, where all of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
than -> from
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
working/bazel/module-structure.md
Outdated
* `flags_test`, which tests that `flags` is working-as-intended. | ||
|
||
This concept already is quite different than a `pub` package, where all of the | ||
files in `lib/` are accessible once y ou have a dependency on that package. In |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spurious space in "y ou"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I have no issue with a document like this if the purpose and scope is clear. |
re: @lrhn's comments, I am taking that is an OK to merge :) Happy to make more edits post-commit. |
I'm not sure the location is great. I see other PRs putting such files into a "background" directory. |
I've added this as a working reference for future language enhancements or experiments that intend to tackle the concept of either packages-in-the-language, modules-in-the-language, visibility restrictions, "friends", etc - i.e. such as #11.