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

Add support for Bazel persistent workers #1024

Merged
merged 1 commit into from Aug 15, 2019

Conversation

@ulysses4ever
Copy link
Contributor

commented Jul 31, 2019

This is the first (working) step with several shortcomings:

  1. The worker binary is assumed to be in ~/bin.
  2. No incremental builds yet.
  3. No tests or documentation.

First should be fixed ASAP, I imagine. I'd appreciate a bit of advice on this particular matter.

Third should be fixed in the due course.

On the positive side: it should support both standard (non-worker) mode as well as the worker one.

@ulysses4ever ulysses4ever requested review from mboes, aherrmann and regnat and removed request for mboes Jul 31, 2019
@ulysses4ever ulysses4ever changed the title WIP: Add support for Bazel persistent workers [WIP] Add support for Bazel persistent workers Jul 31, 2019
@ulysses4ever ulysses4ever force-pushed the worker-release branch from 37bc422 to 0101fa0 Jul 31, 2019
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

How come CI tries to run tests in the worker mode? @regnat It's doomed to fail because the worker is not packaged with rules_haskell yet.

@regnat

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@ulysses4ever according to https://blog.bazel.build/2019/06/19/list-strategy.html

If none of the strategy flags was used, bazel will generate a default list of strategies remote,worker,sandboxed,local and, for every action it wants to execute, will pick up the first strategy that can execute it.

So if it is available, worker is privileged over sandboxed or local. That kinda sucks for us because we can specify a custom --spawn_strategy in rules_haskell's .bazelrc, but all our dependents won't have this and thus will have a weird failure.

Do you think it's possible to have the worker be opt-in in the toolchain declaration? Something like adding a use_worker :: Bool flag to it

@regnat

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Actually running it explicitely without the worker also fails for a different reason:

▶ bazel test //... --spawn_strategy sandboxed
INFO: Invocation ID: 724cf41e-887b-4ee0-8a0f-e7a5e1a77f78
WARNING: /home/regnat/.cache/bazel/_bazel_regnat/9b6df921bb6e07dc34fcea21f9031196/external/zlib.dev/BUILD:9:1: in linkstatic attribute of cc_library rule @zlib.dev//:zlib: setting 'linkstatic=1' is re
commended if there are no object files
INFO: Analyzed 290 targets (0 packages loaded, 0 targets configured).
INFO: Found 196 targets and 94 test targets...
ERROR: /home/regnat/Documents/taff/tweag/bazel/rules_haskell/tests/binary-custom-main/BUILD.bazel:8:1: HaskellBuildBinary //tests/binary-custom-main:binary-custom-main failed (Exit 1) worker failed: e
rror executing command bazel-out/host/bin/external/rules_haskell/haskell/worker bazel-out/k8-fastbuild/bin/tests/binary-custom-main/compile_flags_binary-custom-main_HaskellBuildBinary ... (remaining 1
 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
src/main/tools/linux-sandbox-pid1.cc:427: "execvp(bazel-out/host/bin/external/rules_haskell/haskell/worker, 0x18dc950)": No such file or directory
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

As discussed on today's meeting with @regnat, the flag should be doable.

Strange that I don't get the same error in binary-custom-main as @regnat does. What I get with bazel test //... --spawn_strategy sandboxed -k is:

Executed 35 out of 94 tests: 35 tests pass and 59 fail to build.

And //tests:test-binary-custom-main PASSED.

@ulysses4ever ulysses4ever force-pushed the worker-release branch from 0101fa0 to 898b37a Aug 1, 2019
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

The latest update is not intended to pass through CI yet, but it should be possible to run it locally as shown by @regnat :

bazel test //... --spawn_strategy sandboxed -k

It passes for me.

@ulysses4ever ulysses4ever force-pushed the worker-release branch from 898b37a to 662fc50 Aug 1, 2019
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

I added a toolchain flag to indicate whether we want to use worker. CI should pass now.

@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Could someone help me decipher CI failures, please? Why CirecleCI fails when downloading nix (I guess?)? And what is bazel lint? I cannot run bazel lint locally (Command 'lint' not found.)

@regnat

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Could someone help me decipher CI failures, please? Why CirecleCI fails when downloading nix (I guess?)? And what is bazel lint? I cannot run bazel lint locally (Command 'lint' not found.)

bazel lint is just the name of the test which doesn't map to an existing bazel command.
The actual command run is bazel run //:buildifier which checks the formatting of the starlark files.
You can automatically fix it with bazel run //:buildifier-fix (all that is in the README with a bit more details).

The Circle failure is… interesting. It's definitely not related to your branch and is probably transient. Hopefully that will solve itself on the next CI run 🤞 . Or maybe @Profpatsch has an idea

@ulysses4ever ulysses4ever force-pushed the worker-release branch from 662fc50 to 3e46be0 Aug 2, 2019
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Thanks @regnat!

CI is happy 🎉 Could someone review, please?

@ulysses4ever ulysses4ever changed the title [WIP] Add support for Bazel persistent workers Add support for Bazel persistent workers Aug 2, 2019
@Profpatsch

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Since CI now passes it must have been transient.

Copy link
Member

left a comment

Looks good!

By default this won't affect user's, nonetheless, I think it should be documented in the visible corners that this feature is not ready for consumption, yet.


# Detect if we are in the persistent worker mode
if [ "$2" == "--persistent_worker" ]; then
>&2 echo "Hello worker"

This comment has been minimized.

Copy link
@aherrmann

aherrmann Aug 12, 2019

Member

Looks like debug output that can be removed.

if [ "$2" == "--persistent_worker" ]; then
>&2 echo "Hello worker"
compile_flags=("${compile_flags[@]:1}") # remove ghc executable
exec ~/bin/worker ${compile_flags[@]} --persistent_worker

This comment has been minimized.

Copy link
@aherrmann

aherrmann Aug 12, 2019

Member

Best document that this is a work in progress and add a pointer to the worker repo.

@@ -173,7 +176,8 @@ def haskell_register_ghc_nixpkgs(
locale = None,
repositories = {},
repository = None,
nix_file_content = None):
nix_file_content = None,
use_worker = False):

This comment has been minimized.

Copy link
@aherrmann

aherrmann Aug 12, 2019

Member

This flag will show up in the API docs for master. Best document that this is not ready for use by regular users, yet.

@ulysses4ever ulysses4ever force-pushed the worker-release branch from 3e46be0 to 878982f Aug 15, 2019
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

@aherrmann thanks for the review! I addressed your comments. Could you check it out again?

Copy link
Member

left a comment

Thanks, looks good. One minor change request.

use_worker: This is a part of experimental support for the persistent worker mode.
It is not intended for production usage, yet.
Toolchains can be used to compile Haskell code. To have this

This comment has been minimized.

Copy link
@aherrmann

aherrmann Aug 15, 2019

Member
Suggested change
Toolchains can be used to compile Haskell code. To have this
Toolchains can be used to compile Haskell code. To have this

This comment has been minimized.

Copy link
@ulysses4ever

ulysses4ever Aug 15, 2019

Author Contributor

Wow, @aherrmann, I missed your comment somehow, I'm terribly sorry! I opened #1044 to fix this.

@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Hmm, I wasn't able to use worker when changed the call to haskell_register_ghc_nixpkgs in the top-level WORKSPACE with use_worker=True -- the build proceeded as usual in the "non-worker" mode. Let's probably not merge until I figure it out?

@ulysses4ever ulysses4ever changed the title Add support for Bazel persistent workers [WIP] Add support for Bazel persistent workers Aug 15, 2019
This is the first (working) step with several shortcomings:

* The `worker` binary is assumed to be in ~/bin (or symlinked there).
* No incremental builds yet.

To test the worker: add `use_worker=True` to the call of
`haskell_register_ghc_nixpkgs` in the WORKSPACE file.
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Ok, I figured it and now can run one test with worker successfully. But if I do bazel test //... I get

ERROR: infinite symlink expansion detected
[start of symlink chain]
/home/artem/Dev/rules_haskell
/home/artem/Dev/rules_haskell/examples/bazel-examples/external/rules_haskell
/home/artem/.cache/bazel/_bazel_artem/9f09c111e94c2a1957e003be5e5101a4/execroot/rules_haskell_examples/external/rules_haskell
/home/artem/.cache/bazel/_bazel_artem/9f09c111e94c2a1957e003be5e5101a4/external/rules_haskell
[end of symlink chain]
ERROR: Failed to get information about path, for examples/bazel-examples/external/rules_haskell, skipping: Infinite symlink expansion

Why is that?

@ulysses4ever ulysses4ever force-pushed the worker-release branch from 878982f to 132d0af Aug 15, 2019
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

My problem looks like this: I reference worker binary with ~ ($HOME). But I don't see why 1) I didn't hit it before; 2) it doesn't manifest in singleton tests.

The workaround from the issue thread (--output_base) doesn't help, though.

@ulysses4ever ulysses4ever changed the title [WIP] Add support for Bazel persistent workers Add support for Bazel persistent workers Aug 15, 2019
@ulysses4ever

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

SOmething wrong was with my local copy. Clean checkout works (and tests) as expected: both in standard mode and in the persistent worker mode.

@ulysses4ever ulysses4ever merged commit 22545c5 into master Aug 15, 2019
5 checks passed
5 checks passed
Summary 3 potential rules
Details
build Workflow: build
Details
buildkite/rules-haskell/pr Build #166 passed (19 minutes, 52 seconds)
Details
deploy/netlify Deploy preview ready!
Details
tweag.rules_haskell #20190815.18 succeeded
Details
@ulysses4ever ulysses4ever deleted the worker-release branch Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.