cc_wrapper for all OSes - #1039
Merged
Merged
Conversation
aherrmann
force-pushed
the
cc_wrapper_windows
branch
from
August 16, 2019 07:56
4d5597d to
16bfd12
Compare
Contributor
Author
|
Rebased on master to fix merge conflict. |
Profpatsch
reviewed
Aug 19, 2019
5 tasks
Profpatsch
reviewed
Aug 28, 2019
| # current argument, the stream of up-coming arguments, and a | ||
| # reference to the list of arguments to forward. The handler must | ||
| # return True if it consumes the argument, and return False if | ||
| # another handler should consume the argument. |
Contributor
There was a problem hiding this comment.
This is beautifully bonkers, but I got no suggestions.
aherrmann
force-pushed
the
cc_wrapper_windows
branch
from
August 28, 2019 13:40
2aa93a8 to
b8ef291
Compare
Profpatsch
reviewed
Aug 28, 2019
Always pass locale_archive to run_ghc if provided.
- Shortens library search paths to stay below maximum path length on Windows.
GHC generates library search paths that contain redundant up-level
references (..). This can exceed the maximum path length on Windows, which
will cause linking failures. This wrapper shortens library search paths to
avoid that issue.
- Shortens rpaths and load commands on macOS.
The rpaths and load commands generated by GHC and Bazel can quickly exceed
the MACH-O header size limit on macOS. This wrapper shortens and combines
rpaths and load commands to avoid exceeding that limit.
- Finds .so files if only .dylib are searched on macOS.
Bazel's cc_library will generate .so files for dynamic libraries even on
macOS. GHC strictly expects .dylib files on macOS. This wrapper hooks into
gcc's --print-file-name feature to work around this mismatch in file
extension.
Otherwise we would still require symbolic links for dynamic library dependencies on macOS for REPL targets.
The python cc_wrapper is too slow on Windows. See #1002 Rewriting it in C++ would be a lot of work as we cannot rely on C++17's std::filesystem, yet. Including a C++ filesystem library would impose a large dependency. Fortunately, the Windows cc_wrapper only needs to shorten library paths, not handle dynamic libraries. It is easier to use a dedicated bash cc_wrapper on Windows instead.
These libraries cause linking errors on Windows when linking pthreads.
Use nameref variables to return values from functions instead of echoing strings. This allows to avoid command substitution, which would create a subprocess for each function call, which is costly on Windows.
It's an internal attribute and should not be accessed directly, instead clients should use `Args.linking`, `Args.compiling`, or `Args.printing_file_name`. https://github.com/tweag/rules_haskell/pull/1039/files#r318549754
Use clearer variable names and add comments to improve readability. https://github.com/tweag/rules_haskell/pull/1039/files#r318540827
aherrmann
force-pushed
the
cc_wrapper_windows
branch
from
August 29, 2019 10:18
b8ef291 to
05300f1
Compare
Profpatsch
reviewed
Aug 29, 2019
Profpatsch
approved these changes
Aug 29, 2019
Profpatsch
left a comment
Contributor
There was a problem hiding this comment.
How to eat an elephant.
From my side this is good to go, but it could be good to have a second review, since it’s highly non-trivial code.
aherrmann
force-pushed
the
cc_wrapper_windows
branch
from
August 30, 2019 10:51
02c0379 to
99d601f
Compare
Contributor
Author
|
The CI failure is a timeout. I'm testing locally if there's a performance regression. I also started a rerun. The reason for the timeout may not be that anything in particular got slower, but rather that everything had to be rebuilt. |
Reduces the runtime of the cc_wrapper in nix-shell by 60% in case of short overall cc_wrapper execution time.
aherrmann
force-pushed
the
cc_wrapper_windows
branch
from
September 3, 2019 09:34
4b76ac1 to
a4f40a8
Compare
aherrmann-da
pushed a commit
to digital-asset/daml
that referenced
this pull request
Sep 6, 2019
Updates to latest rules_haskell master and adds the cc_wrapper PR as a patch, see tweag/rules_haskell#1039.
6 tasks
aherrmann-da
pushed a commit
to digital-asset/daml
that referenced
this pull request
Sep 6, 2019
Updates to latest rules_haskell master and adds the cc_wrapper PR as a patch, see tweag/rules_haskell#1039.
mboes
approved these changes
Sep 8, 2019
| ) | ||
|
|
||
| if os == "windows": | ||
| # These libraries cause linking errors on Windows when linking |
Contributor
There was a problem hiding this comment.
Is there an upstream ticket for this?
Contributor
Author
There was a problem hiding this comment.
Not that I know. I'll try to come up with a good repro and open a ticket.
mergify Bot
pushed a commit
to digital-asset/daml
that referenced
this pull request
Sep 9, 2019
* Fix bazel query deps(//...) * Add rules_haskell cc_wrapper Updates to latest rules_haskell master and adds the cc_wrapper PR as a patch, see tweag/rules_haskell#1039. * Shorten include dirs in cc-wrapper When using `haskell_cabal_library` GHC constructs unnecessarily long include directories which can quickly overflow the maximum command-line length. This patch avoids the issue by normalizing include paths. * glob --> breadth_first_walk
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Introduces
cc_wrapperfor all operating systems.GHC generates library search paths that contain redundant up-level references (
..). This can exceed the maximum path length on Windows, which will cause linking failures.The rpaths and load commands generated by GHC and Bazel can quickly exceed the MACH-O header size limit on macOS. This wrapper shortens and combines rpaths and load commands to avoid exceeding that limit.
Bazel's cc_library will generate .so files for dynamic libraries even on macOS. GHC strictly expects .dylib files on macOS. This wrapper hooks into gcc's --print-file-name feature to work around this mismatch in file extension.
This works around py_binary executables can be very slow on Windows bazelbuild/bazel#8981, execution of py_binary targets is too slow on Windows to be practical for cc_wrapper. On Windows the cc_wrapper only needs to shorten library search paths and not worry about rpaths or dynamic library file extensions. This allows for a much simpler cc_wrapper.
Second attempt at #1002, which failed due to py_binary being too slow on Windows, see bazelbuild/bazel#8981.
Closes #958