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.

Sign up for GitHub

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

cabal_wrapper: Bundle all the include_dirs into one #1081

Closed
wants to merge 1 commit into from
Closed

cabal_wrapper: Bundle all the include_dirs into one #1081

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion haskell/private/cabal_wrapper.sh.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,29 @@ while [[ $1 != -- ]]; do
done
shift 1

declare -a include_dirs
declare -a path_args
for arg in $@; do
if [[ $arg =~ --extra-include-dirs=(.*) ]]; then
include_dirs+=("${BASH_REMATCH[1]}")
else
path_args+=("$arg")
fi
done

# Create one directory with a link to all the possibly needed header files.
# This is required to avoid blowing-up the maximum command-line size when
# there are too many include dirs
local_include_dir="$execroot/local-package-db"
mkdir "$local_include_dir"
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/
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, these are a leftover from an old iteration where I tried to do the same thing with package-dbs

Copy link
Member

Choose a reason for hiding this comment

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

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",
 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hell, I though symlink support was already required on windows. I guess that puts an end to this experiment then 😞

done
done
extra_args+=("--extra-include-dirs=$local_include_dir")

ar=$(realpath %{ar})
strip=$(realpath %{strip})
distdir=$(mktemp -d)
Expand Down Expand Up @@ -117,7 +140,7 @@ $execroot/%{runghc} $setup configure \
--package-db=clear \
--package-db=global \
"${extra_args[@]}" \
"${@/=/=$execroot/}" \
"${path_args[@]/=/=$execroot/}" \
--package-db=$package_database # This arg must come last.
$execroot/%{runghc} $setup build --verbose=0 --builddir=$distdir
$execroot/%{runghc} $setup install --verbose=0 --builddir=$distdir
Expand Down