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

Completes the prototype implementation of Bazel persistent worker mode #1055

Merged
merged 2 commits into from
Aug 28, 2019

Conversation

ulysses4ever
Copy link
Collaborator

Started in 132d0af. This commit imports worker's code and sets everything to make
the persistent worker mode available to a regular rules_haskell user.

The worker code lives in tools/worker.

The mode is supported in ghc_nixpkgs-toolchain only, at this point.

@ulysses4ever ulysses4ever changed the title Completes the prototype implementation of Bazel persistent worker mode Complets the prototype implementation of Bazel persistent worker mode Aug 24, 2019
@ulysses4ever ulysses4ever changed the title Complets the prototype implementation of Bazel persistent worker mode Completes the prototype implementation of Bazel persistent worker mode Aug 24, 2019
haskell/private/context.bzl Outdated Show resolved Hide resolved
tests/haskell_proto_library/BUILD.bazel Outdated Show resolved Hide resolved
tests/haskell_proto_library/Bar.hs Outdated Show resolved Hide resolved
tools/worker/Compile.hs Outdated Show resolved Hide resolved
tools/worker/Compile.hs Outdated Show resolved Hide resolved
tools/worker/Main.hs Outdated Show resolved Hide resolved
@ulysses4ever ulysses4ever changed the title Completes the prototype implementation of Bazel persistent worker mode [WIP] Completes the prototype implementation of Bazel persistent worker mode Aug 27, 2019
@ulysses4ever
Copy link
Collaborator Author

Note. CI fails on examples and tutorial because, as it turns out, we accidentally added the requirement to load worker dependencies even when the worker is not used. A solution based on select is currently being crafted.

@ulysses4ever
Copy link
Collaborator Author

Thanks @aherrmann for the review! Hopefully, all the comments are addressed. I also changed the implementation of the worker switch to use select so that we don't require loading worker dependencies when the worker is not used. Please, take a look when have time.

@ulysses4ever ulysses4ever changed the title [WIP] Completes the prototype implementation of Bazel persistent worker mode Completes the prototype implementation of Bazel persistent worker mode Aug 27, 2019
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thank you! Only one minor comment about the rule_type tag.

I've tested this on my machine and bazel test --define use_worker=True --worker_sandboxing //... passes.

haskell/nixpkgs.bzl Show resolved Hide resolved
haskell/defs.bzl Show resolved Hide resolved
haskell/defs.bzl Outdated Show resolved Hide resolved
@ulysses4ever
Copy link
Collaborator Author

@aherrmann thanks!

I've tested this on my machine and bazel test --define use_worker=True --worker_sandboxing //... passes.

Yes, if you enable worker dependencies -- it's all good! But if you don't, plain bazel test //... fails (for me, not sure if it's some local issue). Instead, bazel test //tests/... succeeds. Could you try it? If it fails for you too we might need to fix the README: it mentions bazel test //... now.

@aherrmann
Copy link
Member

Yes, if you enable worker dependencies -- it's all good! But if you don't, plain bazel test //... fails (for me, not sure if it's some local issue). Instead, bazel test //tests/... succeeds. Could you try it?

Yes, same for me. That makes sense as //... includes //tools/worker:bin. bazel test //... will also try to build (not execute) non-test targets. You can observe the same if you turn e.g. //tests/binary-simple:binary-simple into a haskell_binary instead of haskell_test and run bazel test //tests/binary-simple/....

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Aug 27, 2019

Thanks for checking! It doesn't seem entirely reasonable to me to build non-test targets during bazel test but well… I guess I'll change README then.

@ulysses4ever ulysses4ever force-pushed the worker-packaging-release branch 3 times, most recently from 143ded4 to 35a2b55 Compare August 28, 2019 14:26
@ulysses4ever
Copy link
Collaborator Author

Andreas, I tried to address all your comments. What do you think?

tools/worker/Compile.hs Outdated Show resolved Hide resolved
@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Aug 28, 2019

I tried to use “Apply suggestion” button for the first time in my life to see what happens. I wonder if it's possible to squash the resulting tiny commit, on merge.

Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK mergify is not configured to squash-merge.

Thanks for addressing the comments. Looks good to me.

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Aug 28, 2019

I'll squash manually then. What's the proper way to proceed after that? Add the merge-queue label?

@aherrmann aherrmann added the merge-queue merge on green CI label Aug 28, 2019
@ulysses4ever
Copy link
Collaborator Author

The netlify failure doesn't seem related to the PR. I'm confused…

…e support in rules_haskell

Started in 132d0af. This commit imports worker's code and sets everything to make
the persistent worker mode available to a regular `rules_haskell` user.

The worker code lives in `tools/worker`.

The mode is supported in ghc_nixpkgs-toolchain only, at this point.
@mergify mergify bot merged commit a42b91d into master Aug 28, 2019
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ulysses4ever!

I know, it's been a while... but do still have the worker.proto file laying around somewhere? It would be nice if you could provide it to us. 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! I'll look into it today. I'm surprised that something is missing: I'd expect that any missing file should be easily generated or downloaded or something... But it's been four years since I touched it so I may very well be wrong.

It's kinda ironic that a server machine I was using for it (and which I was using since) was recently (this July) decommissioned, and I had to move to another one, and in the process I nuked a lot of stuff. Probably including the playground for this project. But let's be optimistic and wait until I try to look it up locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, it's the generated file that I committed instead of the source. Shame on me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good. I developed this code as a separate repo first: https://github.com/tweag/bazel-worker/ (should have been a git submodule, perhaps, but they're such a pain...) and it does have worker.proto:

Note that it's a file provided by Bazel itself:

It's fun to see the diff between the two: they extended the protocol quite a bit over the years.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nice. Thank you for the swift response! 💯

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.

3 participants