Use more precise compiler flags. #241
Conversation
Previously, if we had a header `foo/bar/baz/header.h` we were passing `-I` for *every* partial path (`-Ifoo -Ifoo/bar -Ifoo/bar/baz`). This is too aggressive; in practice, it's only necessary to specify the "root" directories like `bazel-out/*/genfiles`. The previous behavior led to some problems in Hazel with headers from different sources and the same basename confusing the preprocessor and making it import the wrong one. Fixed by calling `cc.compile_flags` to get the compiler flags directly. Then, pass them to GHC via `-optP` and to hsc2hs via `--cflag`. For reference, this is the implementation of `cc.compile_flags` which indicates it passes preprocessor-related flags (`-D`, `-isystem`, `-iquote` and `-I`): https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkylarkApiProvider.java#L169
|
Looks like this doesn't interact as well with |
This parameter is available for cc_library, and Bazel team has it on their roadmap to add it to `cc_import` as well: bazelbuild/bazel#4748 This change does potentially break existing `cc_import` rules that didn't specify it previously. However, that change seems worth it for being more explicit.
haskell/cc.bzl
Outdated
|
|
||
| return hdrs.to_list(), ["-I" + dir for dir in dirs] | ||
| return hdrs.to_list(), [f for flag in flags for f in ctx.tokenize(flag)] |
There was a problem hiding this comment.
ctx.tokenize does shell tokenization I guess? I could not find documentation about this: https://docs.bazel.build/versions/master/skylark/lib/ctx.html.
haskell/cc.bzl
Outdated
| dirs = set.to_list(set.from_list([hdr.dirname for hdr in hdrs])) | ||
| flags = set.to_list(set.from_list( | ||
| [f for dep in ctx.attr.deps if hasattr(dep, "cc") | ||
| for f in dep.cc.compile_flags])) |
There was a problem hiding this comment.
I'm not sure, maybe forwarding all compile flags of all dependencies at once is an overkill? May be we'll get something unnecessary or conflicting this way?
There was a problem hiding this comment.
See the link in the PR description; in practice this field only actually covers CPP-related pragmas, so I think it provides the right necessary set of flags. (For what it's worth, this is the approach the Haskell rules developed internally at Google have taken, and it looks like it's been stable over the last couple of years.) I'm not aware of a more robust approach, at least until bazelbuild/bazel#2163 is completed.
ctx.tokenize is not officially documented but is defined here: https://github.com/bazelbuild/bazel/blob/abbb9002c41bbd53588e7249756aab236f6fcb4b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleContextApi.java#L290
I could also try seeing how robust it would be to reimplement it from scratch.
There was a problem hiding this comment.
Also note that the cc_library "defines" and "includes" fields already have strong warnings in its docs:
https://docs.bazel.build/versions/master/be/c-cpp.html
For example:
Unlike copts, these flags are added for the target and every rule that depends on it! Be very careful, since this may have far-reaching effects. When in doubt, add "-D" (or /D on Windows) flags to copts instead.
Which indicates that we shouldn't have to worry about conflicts.
Another option is to assemble the set of compile flags manually, basically replicating the logic in compile_flags. For example, in rules_go:
https://github.com/bazelbuild/rules_go/blob/master/go/private/rules/cgo.bzl#L187
Compare:
https://github.com/bazelbuild/bazel/blob/fa135cf600a957cbcef0ca45f97d5a9009d40859/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkylarkApiProvider.java#L175
I don't see this in the diff though. |
|
Updated the PR; it was missing some changes I pushed to the wrong place. |
There was a problem hiding this comment.
This LGTM. I trust that the breaking change in haskell_cc_import (need to use strip_include_prefix now) makes it consistent with the include path at which the cc_library rule (and in future cc_import) make header files available?
In of itself, I'm not sure I like that change. But I do like it a lot if it's to be consistent with core rules.
Rather than calling compile_flags plus `ctx.tokenize`, call `cc.include_directories`, `cc.defines`, etc. directly.
Follow the behavior of `cc_library` more closely; allow either a relative path (relative to the package) or an absolute path (relative to the workspace root).
|
I updated the implementation; please take another look:
|
|
Looks great! |
`-include` is used by Cabal (or build systems that emulate it, like Hazel) to specify macros like `MIN_VERSION_base(...)`. As of tweag#241 we're passing arguments around include directories via `optP`. Since a manual `compiler_flag` call might also specify `-optP`, and need to reference one of those header files, we should pass `compiler_flags` *after* passing the include directories.
Previously, if we had a header
foo/bar/baz/header.hwe were passing-Ifor every partial path (
-Ifoo -Ifoo/bar -Ifoo/bar/baz). This is tooaggressive; in practice, it's only necessary to specify the "root" directories
like
bazel-out/*/genfiles.The previous behavior led to some problems in Hazel with headers from
different sources and the same basename confusing the preprocessor and making it
import the wrong one.
Fixed by calling individual fields of
ccto get the compiler flags directly.Then, pass them to GHC via
-optPand to hsc2hs via--cflag.For reference, this is the same approach as
rules_go:https://github.com/bazelbuild/rules_go/blob/master/go/private/rules/cgo.bzl#L187
Also add a
strip_include_prefixattr tohaskell_cc_import. This parameter is available for cc_library, and Bazel team has it on their roadmap to add it tocc_importas well:bazelbuild/bazel#4748
This change does potentially break existing
cc_importrules that didn't specify it previously. However, it seems worth it to fix the above issue at the cost of being more explicit.