cabal_wrapper: Bundle all the include_dirs into one #1081
Conversation
Avoids some `command-line too long` errors by reducing a lot the number of arguments passed to gcc
There was a problem hiding this comment.
I like the idea, but I'm not sure it can be made to work.
- This doesn't work on Windows because creating symlinks requires special permissions. Relying on
ln -swould make admin or developer mode (requires admin to enable) a strict requirement on Windows. - This fails on collisions.
| chmod +w $local_include_dir | ||
| for db in "${include_dirs[@]}"; do | ||
| for conf_file in $db/*; do | ||
| ln -s "$(realpath $conf_file)" $local_include_dir/ |
There was a problem hiding this comment.
local-package-db, db, and conf_file are misnomers. You can see this by logging the values of $db and $conf_file when building e.g. @stackage-zlib//:zlib:
db=bazel-out/k8-fastbuild/bin/external/zlib.dev/_virtual_includes/zlib
conf_file=bazel-out/k8-fastbuild/bin/external/zlib.dev/_virtual_includes/zlib/zconf.h
conf_file=bazel-out/k8-fastbuild/bin/external/zlib.dev/_virtual_includes/zlib/zlib.h
conf_file=bazel-out/k8-fastbuild/bin/external/zlib.dev/_virtual_includes
They are include directories and header files and not package-db or package configuration files.
There was a problem hiding this comment.
Indeed, these are a leftover from an old iteration where I tried to do the same thing with package-dbs
| chmod +w $local_include_dir | ||
| for db in "${include_dirs[@]}"; do | ||
| for conf_file in $db/*; do | ||
| ln -s "$(realpath $conf_file)" $local_include_dir/ |
There was a problem hiding this comment.
This can lead to collisions. E.g. adding a cc_library dependency to //tests/haskell_cabal_library:lib as in the patch below causes the following error:
ln: failed to create symbolic link '/home/aj/.cache/bazel/_bazel_aj/6cfdc159761b06fafca60627831d1084/sandbox/linux-sandbox/30/execroot/rules_haskell/local-package-db/tests': File exists
Target //tests/haskell_cabal_library:lib failed to build
The reason is that we're receiving the flags --extra-include-dirs=. and --extra-include-dirs=bazel-out/k8-fastbuild/bin' and both those directories contain the directory tests.
patch
diff --git a/tests/haskell_cabal_library/BUILD.bazel b/tests/haskell_cabal_library/BUILD.bazel
index 03b6912b..d603da01 100644
--- a/tests/haskell_cabal_library/BUILD.bazel
+++ b/tests/haskell_cabal_library/BUILD.bazel
@@ -7,6 +7,8 @@ load(
haskell_toolchain_library(name = "base")
+cc_library(name = "c-lib")
+
haskell_cabal_library(
name = "lib",
srcs = [
@@ -17,6 +19,7 @@ haskell_cabal_library(
"use-base",
"expose-lib",
],
+ deps = [":c-lib"],
version = "0.1.0.0",
)There was a problem hiding this comment.
Can't this be solved by simply ignoring the error or overwriting the previous one ? I assume that if there's a collision in the available includes then one of them must take precedence (though I don't know whether it is specified somewhere which one should be taken into account)
There was a problem hiding this comment.
I think it would be more like a union of the directory trees. I think in this example . would contain source header files and bazel-out/k8-fastbuild/bin generated headers. In a sequence of -I flags it would be the left-biased union of the directory trees. Things get more complicated when -iquote, -isystem, and -idirafter get involved, but that shouldn't matter for --extra-include-dirs. AFAIK those all translate to -I.
| chmod +w $local_include_dir | ||
| for db in "${include_dirs[@]}"; do | ||
| for conf_file in $db/*; do | ||
| ln -s "$(realpath $conf_file)" $local_include_dir/ |
There was a problem hiding this comment.
On Windows this causes errors of the form
ln: failed to create symbolic link '/c/users/.../rules_haskell/local-package-db/amiga`: Permission denied
E.g. when rebasing on #1074 and building @stackage-zlib//:zlib.
Creating symbolic links requires special permissions on Windows.
There was a problem hiding this comment.
Hell, I though symlink support was already required on windows. I guess that puts an end to this experiment then 😞
|
Closing since this can't work on windows in the general case |
Avoids some
command-line too longerrors by reducing a lot the number of arguments passed to gcc