Skip to content

Combined cc_wrapper for all OSes - #1002

Closed
aherrmann wants to merge 10 commits into
masterfrom
cc_wrapper
Closed

Combined cc_wrapper for all OSes#1002
aherrmann wants to merge 10 commits into
masterfrom
cc_wrapper

Conversation

@aherrmann

@aherrmann aherrmann commented Jul 23, 2019

Copy link
Copy Markdown
Contributor

Replaces the macOS specific CC wrapper by one applicable to all OSes

  • 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.

The CC wrapper is also used for REPL and runghc targets so that these also don't require symlinks for dynamic libraries on macOS.

Closes #958

aherrmann added 5 commits July 23, 2019 16:46
@aherrmann
Windows use cc.tools.cc
9470352
@aherrmann
run_ghc: locale_archive as default input
f95b650 
Always pass locale_archive to run_ghc if provided.
@aherrmann
Combined cc_wrapper for Windows and Unix
b670fb5 
- 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.
@aherrmann
Remove unused osx_cc_wrapper
261de8c
@aherrmann
Use cc_wrapper in ghci
36ade3d 
Otherwise we would still require symbolic links for dynamic library
dependencies on macOS for REPL targets.
@aherrmann
aherrmann requested review from guibou and thufschmitt July 23, 2019 15:03
@aherrmann

aherrmann commented Jul 25, 2019

Copy link
Copy Markdown
Contributor Author

The CI failure on Windows is due to timeout.

Tracing the build of @haskell_vector//:lib on a local Windows machine using procmon (Windows defender disabled) shows the following kind of process tree (simplified for readability).

cc_wrapper.exe
├── Conhost.exe
└── python.exe        (command: python.exe .../cc_wrapper.zip ...)          (time: 10:05:04 - 10:05:13)
    └── python.exe    (command: python.exe .../cc_wrapper-source.py ...)    (time: 10:05:11 - 10:05:11)
        └── gcc.exe
            ...

The first call to python.exe is given a zip archive of the cc_wrapper py_binary target and unpacks it into a temporary directory. The second call to python.exe calls the unpacked main python module. The timings show that the unzipping and clean-up of the temporary files take up the vast majority of the ~9s execution time. This overhead is incurred on every single call to cc_wrapper that GHC performs. On CI it adds up to exceed the timeout.

@aherrmann

aherrmann commented Jul 25, 2019

Copy link
Copy Markdown
Contributor Author

A quick test shows that a cc_wrapper written in C++ and built as cc_binary does not incur this overhead. Rewriting the cc_wrapper in C++ would be a possible work around.

@aherrmann aherrmann mentioned this pull request Jul 25, 2019
Closed
@Profpatsch

Profpatsch commented Jul 25, 2019

Copy link
Copy Markdown
Contributor

Do you plan on doing the rewrite and opening a new PR once that is done? Or do you want to push the rewrite to this PR?

@aherrmann

aherrmann commented Jul 25, 2019

Copy link
Copy Markdown
Contributor Author

Happy to do it in a separate PR. I'll close this for now. I'll be gone the next two weeks, so I won't be able to resolve this before I'm back.

@aherrmann aherrmann closed this Jul 25, 2019
aherrmann added a commit that referenced this pull request Aug 13, 2019
@aherrmann
Windows: cc_wrapper in bash
5255cda 
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.
aherrmann added a commit that referenced this pull request Aug 13, 2019
@aherrmann
Windows: cc_wrapper in bash
a6122ee 
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.
aherrmann added a commit that referenced this pull request Aug 13, 2019
@aherrmann
Windows: cc_wrapper in bash
132ed4b 
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.
aherrmann added a commit that referenced this pull request Aug 14, 2019
@aherrmann
Windows: cc_wrapper in bash
66187ee 
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.
@aherrmann aherrmann mentioned this pull request Aug 14, 2019
Merged
aherrmann added a commit that referenced this pull request Aug 16, 2019
@aherrmann
Windows: cc_wrapper in bash
2a1584c 
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.
aherrmann added a commit that referenced this pull request Aug 29, 2019
@aherrmann
Windows: cc_wrapper in bash
f4301ca 
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.
mergify Bot pushed a commit that referenced this pull request Sep 10, 2019
@aherrmann @mergify
Windows: cc_wrapper in bash
c06ae83 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guibou guibou Awaiting requested review from guibou
@thufschmitt thufschmitt Awaiting requested review from thufschmitt

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

symlink_dynamic_library takes an unreasonable amount of time

2 participants

@aherrmann @Profpatsch