Refactoring: remove visible-binaries #557
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6d5393b to
7f16f6e
Compare
gdeest
reviewed
Jan 14, 2019
| ), | ||
| ]), | ||
| outputs = [doctest_log], | ||
| mnemonic = "HaskellDoctest", | ||
| progress_message = "HaskellDoctest {}".format(ctx.label), | ||
| command = """ | ||
| {doctest} "$@" $(cat {module_list} | tr , ' ') > {output} 2>&1 || rc=$? && cat {output} && exit $rc | ||
| """.format(doctest = toolchain.doctest[0].path, output = doctest_log.path, module_list = exposed_modules_file.path), | ||
| {env} |
There was a problem hiding this comment.
What's the reason not to use run_shell's env parameter ?
haskell/haddock.bzl
Outdated
| @@ -89,6 +89,25 @@ def _haskell_doc_aspect_impl(target, ctx): | |||
| depset([hs.toolchain.locale_archive]) if hs.toolchain.locale_archive != None else depset() | |||
| ) | |||
|
|
|||
| # TODO(mboes): we should be able to instantiate this template only | |||
| # once per toolchain instance, rather than here. | |||
| haddock_wrapper = ctx.actions.declare_file("haddock_wrapper") | |||
There was a problem hiding this comment.
Don't we run into the risk of multiple targets declaring the same file ? E.g., if we have two libraries in the same package.
There was a problem hiding this comment.
That's a good point. Thanks for catching that. We should make the name target dependent, much like we already do for object files.
| arguments = [ | ||
| prebuilt_deps, | ||
| args, | ||
| ghc_args, | ||
| ], | ||
| env = hs.env, | ||
| use_default_shell_env = True, |
There was a problem hiding this comment.
Because the haddock wrapper script use mktemp and rmdir. Previously, those were "visible binaries". Now, they are taken from the default shell environment (which Bazel will be restricting soon, by flipping --experimental_strict_action_env on by default).
| @@ -86,38 +81,33 @@ def _haskell_toolchain_impl(ctx): | |||
| ghc_binaries = {} | |||
| for tool in ctx.files.tools: | |||
| if tool.basename in _GHC_BINARIES: | |||
| ghc_binaries[tool.basename] = tool.path | |||
| ghc_binaries[tool.basename] = tool | |||
There was a problem hiding this comment.
So obvious in retrospect, it kinda hurts !
To precisely control what binaries are available during compilation, we used to create a single directory called `visible-binaries`, in which we put all the binaries (ghc, haddock, etc) and shell commands (cat, mkdir, etc) that we needed. There was another point to this: build actions could call any binary and that binary could in turn exec any other binary, since `visible-binaries` was in the `PATH`. In this commit, we do away with that. The reason is that there are downsides to this approach: * there is little value in trying to be more hermetic than Bazel is when it comes to shell commands; * all build actions were reusing the same `PATH`, so we were actually exposing more binaries than strictly necessary (each action needs a different subset); * the logic to create `visible-binaries` was pretty complicated, and is now all gone, hence the net code reduction; * crucially, the `visible-binaries` is not going to work on Windows, because no symlinks on that platform and copying doesn't always work (due to limitations of GHC). With this PR merged in, porting to Windows should now be significantly easier. The downside is the following *breaking change*: we have lost the `extra_binaries` field in rules. But that field was introduced for unusual needs that inline-java has, and was a suspicious feature to begin with. Probably fixes #244.
7f16f6e to
93e1f6f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To precisely control what binaries are available during compilation,
we used to create a single directory called
visible-binaries, inwhich we put all the binaries (ghc, haddock, etc) and shell
commands (cat, mkdir, etc) that we needed. There was another point to
this: build actions could call any binary and that binary could in
turn exec any other binary, since
visible-binarieswas in thePATH.In this commit, we do away with that. The reason is that there are
downsides to this approach:
when it comes to shell commands;
PATH, so we were actuallyexposing more binaries than strictly necessary (each action needs
a different subset);
visible-binarieswas pretty complicated, andis now all gone, hence the net code reduction;
visible-binariesis not going to work on Windows,because no symlinks on that platform and copying doesn't always work
(due to limitations of GHC).
With this PR merged in, porting to Windows should now be significantly
easier. The downside is the following breaking change: we have lost
the
extra_binariesfield in rules. But that field was introduced forunusual needs that inline-java has, and was a suspicious feature to
begin with.
Probably fixes #244.
Closes #512.
cc @gdeest @nasm @gleber for the stepping stone towards Windows support.
cc @ahermann about the dropped support for
extra_binaries.