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

The system for specifying Bazel's distribution dependencies is too complex #12081

Closed
aiuto opened this issue Sep 10, 2020 · 5 comments
Closed
Assignees
Labels
area-EngProd Bazel CI, infrastructure, bootstrapping, release, and distribution tooling P2 We'll consider working on this in future. (Assignee optional) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: feature request

Comments

@aiuto
Copy link
Contributor

aiuto commented Sep 10, 2020

The scheme we use is needlessly complex, which makes it error prone. If you update a distribution dependency you might have to change a file name in 10 places. Using pkg_name for the archive name, and pkg_sha for the sha256 of it, we have.

distdir_tar(
  name = "additional_distfiles",
  archive = [...  pkg_name.tgz, ...]
  sha256 = { ..., pkg_name.tgz: pkg_sha },
  urls = {... pkg_name.tgz: [  http://mirror/....pkg_name.tgz, http://canonical_host/.../pkg_name.tgz ], ...},
)

Then, sometimes we repeat it again in distdir_tar(name = "test_WORKSPACE_files", ...)
or we use the name, sha256, and urls directly in an http_archive rule.

We could make this easier to update by consolidating the information into a single structure, something like

archives = {
   archive_1: { file_name: pkg_name.tgz, sha256: sha, url: [urls] }
   archive_2: ...
}

distdir_tar would change to take a list of keys from the archive rather than 3 aligned attributes (archive, sha26, urls)
instead of http_archive, we would use a wrapper method that just takes the archive name.

Inspired by #12077

@aiuto aiuto added area-EngProd Bazel CI, infrastructure, bootstrapping, release, and distribution tooling untriaged labels Sep 10, 2020
@jin jin added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Dec 4, 2020
@aiuto aiuto self-assigned this Dec 8, 2020
@philwo philwo added P2 We'll consider working on this in future. (Assignee optional) type: bug type: feature request and removed untriaged type: bug labels Dec 8, 2020
bazel-io pushed a commit that referenced this issue Dec 9, 2020
This creates a mechanism to address #12081, but does not convert all use cases yet.

The basic idea is to change distdir_tar so that each dependency is defined in a block rather than split into distinct dictionaries for archive, sha256, and urls.  This means that updating a dependency can be isolated to 3 or 4 lines which appear together, rather than being sprinkled throughout WORKSPACE.

The next steps would be
- do the same for the other dependencies. I want to see this land and stay that way before doing the rest.  Those PRs should have trivial reviews.
- determine how to use DISTDIR_DEPS in the other embedded workspace files, such as src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/cc_configure.WORKSPACE
  - we need to splice distdir_deps.bzl into the dynamically created WORKSPACE files
  - or, export distdir_deps.bzl to the workspace and load it
  - or, since cc_configure.WORKSPACE is data, we generate it at build time.

These could be done in either order.

*other potential changes*

- streamline the http_archive and maybe instances to read directly from DISTDIR. This would eliminate the tedious pattern of
```
    sha256 = DIST_DEPS["rules_pkg"]["sha256"],
    urls = DIST_DEPS["rules_pkg"]["urls"],
```
- hide DISTDIR_DEPS from WORKSPACE.
  - pass just a list of package names (not archive names) to distfile_tar().
  - distdir.bzl loads the deps list directly and looks up data by package name

RELNOTES: None
PiperOrigin-RevId: 346580497
aiuto added a commit to aiuto/bazel that referenced this issue Dec 18, 2020
Create a capability to generate WORKSPACE files for tests at build time,
based on the repository definitions in distdir_deps.bzl.

This makes it possible for the versions of repositories used in tests
to align with the versions used a Bazel build time without upddating
files scattered throughout the source and test trees.

Demonstrated with rules_cc.
@aiuto
Copy link
Contributor Author

aiuto commented Dec 23, 2020

Current plan:

  1. Make create a macro that does an http_archive for a repository from DIST_DEPS Add dist_http_archive() to make it easier to synchronize distdir with http_archive #12722
    This reduces error-prone boilerplate from the top level WORKSPACE
  2. Generate embedded *.WORKSPACE files which must be in sync with dist_deps. Generate rules/cpp/cc_configure.WORKSPACE from distdir_deps.bzl rather than keeping it in sync by hand #12743
    Example: src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/cc_configure.WORKSPACE
    Add a rule that can create WORKSPACE stanzas from a template, using references to repositories in DIST_DEPS.
  3. Iterate on dependencies until all elements //:additional_distfiles are moved to the DIST_DEPS list.

bazel-io pushed a commit that referenced this issue Jan 5, 2021
… http_archive

More for #12081

For dependencies specified in distdir.bzl, we can now include them in Bazel's WORKSPACE with a simpler invocation that can not go out of sync.  For example, this

```
dist_http_archive(
  name = "io_bazel_rules_sass",
)
```
instead of
```
http_archive(
  name = "io_bazel_rules_sass",
  sha256 = "c78be58f5e0a29a04686b628cf54faaee0094322ae0ac99da5a8a8afca59a647",
  strip_prefix = "rules_sass-1.25.0",
  urls = [
    "https://mirror.bazel.build/github.com/bazelbuild/rules_sass/archive/1.25.0.zip",
    "https://github.com/bazelbuild/rules_sass/archive/1.25.0.zip",
  ],
)
```

Also, convert two more packages only used at Bazel build time to the new style.
Next steps:
- convert other repositories which are not needed at run time to this format
- add ability to generate WORKSPACE files used by tests from DISTDIR #12734
- convert rules libraries to this format

Closes #12722.

PiperOrigin-RevId: 350140996
bazel-io pushed a commit that referenced this issue Jan 7, 2021
…r than keeping it in sync by hand

This makes it possible for the versions of repositories used in the distributed Bazel tests to align with the versions used at Bazel build time without updating files scattered throughout the source and test trees.

- Add //distdir_deps.bzl%gen_workspace_stanza rule to generate WORKSPACE rules for libraries in DISTDIR_DEPS.
- Apply the new capability to rules_cc.
- move dist_http_archive from distdir_deps.bzl to distdir.bzl so that all the repository time rules are distinct from the build time rules.

This finalizes the structural changes needed for issue #12081. The remaining part is applying the same pattern for each of the dependencies.

Closes #12743.

PiperOrigin-RevId: 350527359
@aiuto
Copy link
Contributor Author

aiuto commented Jan 7, 2021

Status update: I figured out the pattern for generating files like cc_configure.WORKSPACE. Now that that PR is in place, I can continue for Java.

bazel-io pushed a commit that referenced this issue Jan 8, 2021
This consolidates the specification of the rules_java we depend on into a single place. We generate `http_archive()` stanzas for the main WORKSPACE as well as test harnesses from that, rather than keeping 3 instances in sync by hand. The old practice was error prone because you had to know about all the locations, and an update to one might not break the others immediately, but could be a lurking alignment problem.

This continues the work on #12081.

Closes #12787.

PiperOrigin-RevId: 350761327
@aiuto
Copy link
Contributor Author

aiuto commented Jan 9, 2021

Next round: #12779

aiuto added a commit to aiuto/bazel that referenced this issue Jan 13, 2021
- generate @rules_proto workspace inclusions in tests
- src/test/java/com/google/devtools/build/lib/blackbox/framework/BlackBoxTestEnvironment.java will be done in a future CL. That will be the first case where we need to generate Java code. It requires some design up front.

Context: bazelbuild#12081
bazel-io pushed a commit that referenced this issue Jan 14, 2021
- generate @rules_proto workspace inclusions in shell tests
- generate @rules_proto workspace inclusions in jdk.WORKSPACE
- src/test/java/com/google/devtools/build/lib/blackbox/framework/BlackBoxTestEnvironment.java will be done in a future CL. That will be the first case where we need to generate Java code. It requires some design up front.

Context: #12081

Closes #12825.

PiperOrigin-RevId: 351797952
bazel-io pushed a commit that referenced this issue Jan 15, 2021
Remove the alternate name 'remote_coverage_tools_for_testing'

The overall impact is that when we update coverage_output_generator
we only have to update the URLs and SHA in a single code location.

github bug: #12081

RELNOTES: None
PiperOrigin-RevId: 351999560
@aiuto
Copy link
Contributor Author

aiuto commented Jan 22, 2021

Status update

  • most dependencies required in the distribution tarball have been converted to the new system.
  • Java JDKs remain to be done, I am not an expert,so I am handing that to @comius

@aiuto
Copy link
Contributor Author

aiuto commented Jun 7, 2022

I am closing this out as done enough, based on the status update from 2021-01-21. We can move the few individual JDKS we need as we encounter them, but it is likely we can just leapfrog to bzlmod based deps.

@aiuto aiuto closed this as completed Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-EngProd Bazel CI, infrastructure, bootstrapping, release, and distribution tooling P2 We'll consider working on this in future. (Assignee optional) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: feature request
Projects
None yet
Development

No branches or pull requests

4 participants