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

Simplify clodl implementation #36

Merged
merged 11 commits into from
Dec 14, 2020
Merged

Simplify clodl implementation #36

merged 11 commits into from
Dec 14, 2020

Conversation

facundominguez
Copy link
Member

@facundominguez facundominguez commented Dec 11, 2020

So far, library_closure was implemented with a bunch of rules each trying to do a small piece of work. It turns out, however, that the composition of these rules introduced more issues than it solved.

  1. There was a rule to rename inputs so they looked like libraries to the cc_binary rule that we had.
  2. There was a rule to collect the paths of dependencies of a set of source libraries.
  3. There was a rule to transform the collected paths into a couple of files with arguments for the linker.
  4. There was a rule to output the runfiles of the source libraries.
  5. There was a cc_binary rule to link all the runfiles and the source libraries.
  6. There was a final rule to put all the libraries in a zip file.

Each step had its own set of quirks.

  • cc_binary wouldn't accept files to link that didn't end in .o or .so. It rejects binaries in particular, which motivated rule (1).
  • The paths that rule (2) collected pointed sometimes to the nix store, sometimes to the bazel cache, and sometimes to local sandboxes which would become invalid in subsequent rules.
  • Rule (4) would fail if the source libraries had no runfiles, because then bazel would complain that the rule produced no outputs.
  • Rule (5) cc_binary, required the libraries to link as inputs, but since they were discovered at execution time, it was impossible to plug the inputs to it. We therefore gave the paths to the libraries in files produced by rule (3), and hoped that the paths would still be valid when the linker reached for them.

All these quirks together made challenging to fix an issue without introducing new ones. Therefore this PR, which implements library_closure on a single rule.

Firstly, there is some setup to collect the paths to required tools like the C compiler, scanelf and ldd. Then we collect the dependencies, link them together, and build the zip file in a single run_shell action. Thus, no bazel constraint crops up between these tasks. There is no longer a composition problem.

Additionally, I dropped support for the outzip attribute of library_closure that would allow the caller to choose the name of the output zip file. This was a feature supporting sparkle that isn't hard to address in sparkle itself.

@facundominguez
Copy link
Member Author

cc @mboes

@facundominguez
Copy link
Member Author

I tested this with sparkle already and it works.

clodl/clodl.bzl Outdated Show resolved Hide resolved
clodl/clodl.bzl Show resolved Hide resolved
clodl/clodl.bzl Outdated Show resolved Hide resolved
clodl/clodl.bzl Outdated Show resolved Hide resolved
clodl/clodl.bzl Show resolved Hide resolved
clodl/clodl.bzl Outdated Show resolved Hide resolved
shell.nix Outdated Show resolved Hide resolved
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.

2 participants