Skip to content

Use locally built dpkg_parser instead of downloading pre-built remote binary - #477

Merged
chanseokoh merged 6 commits into
masterfrom
no-online-dpkg_parser
Apr 16, 2020
Merged

Use locally built dpkg_parser instead of downloading pre-built remote binary#477
chanseokoh merged 6 commits into
masterfrom
no-online-dpkg_parser

Conversation

@chanseokoh

@chanseokoh chanseokoh commented Feb 26, 2020

Copy link
Copy Markdown
Member

Resolves some issues in #434.

Basically, implementing #463 (comment)

We are downloading a pre-built dpkg_parser.par, which has caused a lot of issues to us and many external users: scionproto/scion#3673, bazelbuild/bazel#10742 (comment), #434, #474, #472, #471, #463 (comment), #469 (comment), #473

Not only that, it becomes very cumbersome whenever we need to update dpkg_parser and test it. Currently, the process is

  1. File a PR after modifying dpkg_parser.
  2. Merge the PR to trigger an automatic Cloud Build which builds and pushes a new binary into a GCS bucket.
  3. Look into the GCS bucket and search for the new binary by the most recent git commit.
  4. Compute the SHA256 of the new binary.
  5. Update the binary URL and the SHA in package_manager/package_manager.bzl.
  6. File another PR and merge it.

But perhaps one of the most critical issues is that the pre-built binary may not run on users' environments (primarily on Mac as in #434).


More Context About the Issue

I asked the following question on the Bazel Slack Channel.

Our "Distroless" project has a kind of circular dependency.

  • A python binary is required to define a repository_rule. (I guess the rule will be evaluated in the Bazel loading phase?) The python binary is executed to make some files available as output of the rule (via export_files).
  • However, this python binary is actually part of the "Distroless" project itself. It is defined as par_binary and is supposed to be built in the normal execution phase. At least, this par_binary doesn't depend on the repository_rule above, so it can be built first alone.

So, in other words, this python binary is really like an external dependency repo that can exist elsewhere on its own. But for now, it is embedded in the project. What we've been doing is

  1. Build the python binary first in a separate step (bazel build //:my_python_binary.par). Then we manually upload the binary to the Internet.
  2. As the second bazel build //:final_target step, we make the binary available (downloading with http_file) and use it to define a repository_rule.

Now we want to remove the "uploading" and "downloading" hackery. That is, I want Bazel to somehow build the python binary first and then use it to define a repository_rule in a single build step. Still, the only option I can think of is to run two separate build steps.

  1. Run bazel build //:my_python_binary.par first as a separate step.
  2. As the second bazel build //:final_target step, repository_rule picks up the locally built binary at <WORKSPACE>/bazel-bin/my_python_binary.par and use it.

Is it possible to make this work with a single bazel build run? What are other options?

@googlebot googlebot added the cla: yes CLAs look good label Feb 26, 2020
Comment thread CONTRIBUTING.md
@chanseokoh

chanseokoh commented Mar 1, 2020

Copy link
Copy Markdown
Member Author

@yashbhutwala reported that, on Mac, this PR builds a local dpkg_parser with #!/usr/bin/env python shebang, but dpkg_parser.par still ends up #!/usr/bin/python3.

> less bazel-bin/package_manager/dpkg_parser
#!/usr/bin/env python
> less bazel-bin/package_manager/dpkg_parser.par
#!/usr/bin/python3

The user ended up explicitly setting compiler_args = ["--interpreter", "/usr/bin/env python"] in the par_binary rule for dpkg_parser (as explained in google/subpar#87).

Apparently, this PR doesn't resolve #434 completely. However, this will enable fixing #434 when we add the complier_args attribute above.

@yashbhutwala

Copy link
Copy Markdown

@chanseokoh can we make a note about run on mac os using the flag --cpu=darwin?

@chanseokoh

Copy link
Copy Markdown
Member Author

@yashbhutwala I think I need to confirm first if --cpu=darwin is really needed all the time. I know a Mac user and they don't have this problem. I wonder if the error is only specific to you.

Historically, if there's any issue, it is reported pretty quickly, so I plan to wait a bit until someone else hits the CPU error.

@briandealwis

Copy link
Copy Markdown
Member

Odd: I remember having to use --cpu=darwin previously too. But I haven't used it in a while. Not sure why.

@Sineaggi

Sineaggi commented Mar 2, 2020

Copy link
Copy Markdown
Contributor

I maybe be losing my mind, but I believe this showed up in some container tests.

Comment thread package_manager/dpkg.bzl
Comment thread test.sh
@chanseokoh

Copy link
Copy Markdown
Member Author

I maybe be losing my mind, but I believe this showed up in some container tests.

@Sineaggi you mean --cpu=darwin, right? If you still encounter or remember the issue, please file a new issue with details.

@Sineaggi

Sineaggi commented Mar 2, 2020

Copy link
Copy Markdown
Contributor

@chanseokoh yeah, pretty simple to repro on my Mac.

FAIL: //base:base_release_debian10_test (see /private/var/tmp/_bazel_cwalker/f470a75b3d336190f9075654c734a68a/execroot/distroless/bazel-out/k8-py2-fastbuild/testlogs/base/base_release_debian10_test/test.log)
INFO: From Testing //base:base_release_debian10_test:
==================== Test output for //base:base_release_debian10_test:
+ base/base_release_debian10_test.image --norun
Loaded image ID: sha256:8c791177af02ad2d3c3f2808171f5d45a40a338b1d0024f8c6900ad6f4b5fe52
Tagging 8c791177af02ad2d3c3f2808171f5d45a40a338b1d0024f8c6900ad6f4b5fe52 as basebase_debian10:intermediate
+ ../structure_test_linux/file/downloaded version
/private/var/tmp/_bazel_cwalker/f470a75b3d336190f9075654c734a68a/sandbox/darwin-sandbox/25/execroot/distroless/bazel-out/k8-py2-fastbuild/bin/base/base_release_debian10_test.runfiles/distroless/base/base_release_debian10_test: line 7: ../structure_test_linux/file/downloaded: cannot execute binary file
================================================================================

Forcing --cpu=darwin lets the tests pass.

~/Source/distroless (git)-[no-online-dpkg_parser] % bazel test --host_force_python=PY2 --curses=no --test_output=errors  //base:base_release_debian10_test --cpu=darwin --python_version=PY2
Loading:
Loading: 0 packages loaded
INFO: Build option --cpu has changed, discarding analysis cache.
Analyzing: target //base:base_release_debian10_test (0 packages loaded, 0 targets configured)
INFO: Analyzed target //base:base_release_debian10_test (1 packages loaded, 7050 targets configured).
INFO: Found 1 test target...
[0 / 3] [Prepa] BazelWorkspaceStatusAction stable-status.txt
INFO: From ImageLayer base/static_debian10-layer.tar:
Duplicate file in archive: ./etc/os-release, picking first occurrence
[41 / 47] ImageLayer base/base_debian10-layer.tar; 1s darwin-sandbox ... (2 actions, 1 running)
[47 / 48] Testing //base:base_release_debian10_test; 0s darwin-sandbox
Target //base:base_release_debian10_test up-to-date:
  bazel-bin/base/base_release_debian10_test
INFO: Elapsed time: 3.179s, Critical Path: 2.42s
INFO: 12 processes: 12 darwin-sandbox.
INFO: Build completed successfully, 21 total actions
//base:base_release_debian10_test                                        PASSED in 0.8s

Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
INFO: Build completed successfully, 21 total actions

@Sineaggi

Sineaggi commented Mar 2, 2020

Copy link
Copy Markdown
Contributor

Otherwise, I've pulled the changes down, kept the compiler_args change, used cpu=darwin, and the whole project builds. Looks good!

@briandealwis

Copy link
Copy Markdown
Member

Oh that's right — for container-structure-test 🤦‍♂

Comment thread package_manager/cloudbuild.yaml
@chanseokoh

Copy link
Copy Markdown
Member Author

@nlopezgi agreed that this is a common problem where they have never found a good solution, other repos also have this sort of thing, mostly the changes in this PR seems like pretty standard hacks that you have to make repo rules work, and the PR looks fine in general.

lukedirtwalker added a commit to lukedirtwalker/scion that referenced this pull request Aug 11, 2020
We saw many issues during downloading debian snapshots,
hopefully this new version works better.

Unfortunately to run distroless we need to manually build the dpkg_parser,
see GoogleContainerTools/distroless#477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes CLAs look good

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants