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.

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

inclusion validation fails when dot-d files have absolute paths for transitive #includes #14346

Closed
amberdixon opened this issue Nov 30, 2021 · 13 comments · May be fixed by #14364
Closed

inclusion validation fails when dot-d files have absolute paths for transitive #includes #14346

amberdixon opened this issue Nov 30, 2021 · 13 comments · May be fixed by #14364
Labels
team-Rules-CPP Issues for C++ rules untriaged

Comments

@amberdixon
Copy link

Description of the problem / feature request:

In some cases, clang outputs absolute paths to the dot-d files for objective-c source files. The bazel5 code that detects inclusion problems with Objective-C source files fails.

Feature requests: what underlying problem are you trying to solve with this feature?

One of my objective-c libraries has source files whose corresponding .d files contain absolute paths.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

  1. Clone and check out the rules_ios WIP PR here: Repro problem with absolute paths in dot-d files breaking inclusion validation in bazel5 bazel-ios/rules_ios#366
  2. Run bazel build -s //tests/ios/InclusionProblemRepro:LibraryA_objc

Expected result: build succeeds
Actual: Build fails with the error message: ERROR: /Users/amberdixon/Development/rules_ios/tests/ios/InclusionProblemRepro/BUILD.bazel:3:14: Compiling tests/ios/InclusionProblemRepro/Classes/Extensions/D.m failed: undeclared inclusion(s) in rule '//tests/ios/InclusionProblemRepro:LibraryA_objc': this rule is missing dependency declarations for the following files included by 'tests/ios/InclusionProblemRepro/Classes/Extensions/D.m': '/private/var/tmp/_bazel_amberdixon/bee3f665d317c886aa3a5be8caf5517a/sandbox/darwin-sandbox/12/execroot/build_bazel_rules_ios/tests/ios/InclusionProblemRepro/Classes/HeaderB.h' Target //tests/ios/InclusionProblemRepro:LibraryA_objc failed to build

Also note that the .d file for this source has an absolute path:

~/Development/rules_ios amber/repro-inclusion-issue-bazel5 tail -2  bazel-out/applebin_macos-darwin_x86_64-dbg-ST-45eefeec3d2c/bin/tests/ios/InclusionProblemRepro/_objs/LibraryA_objc/arc/D.d 
  tests/ios/InclusionProblemRepro/Classes/HeaderC.h \
  /private/var/tmp/_bazel_amberdixon/bee3f665d317c886aa3a5be8caf5517a/sandbox/darwin-sandbox/12/execroot/build_bazel_rules_ios/tests/ios/InclusionProblemRepro/Classes/HeaderB.h

What operating system are you running Bazel on?

MacOS 11.6.1

What's the output of bazel info release?

development version

If bazel info release returns "development version" or "(@non-git)", tell us how you built Bazel.

Cherry-pick this commit (d966a0d) onto the 5.0.0-pre.20211011.2 bazel pre-release and rebuild bazel.

What's the output of git remote get-url origin ; git rev-parse master ; git rev-parse HEAD ?

~/Development/bazel (15a1b19085...) git remote get-url origin ; git rev-parse master ; git rev-parse HEAD
[email protected]:bazelbuild/bazel.git
9108f9b
15a1b19085ee7c423a20d98590f980abf35a5805

Have you found anything relevant by searching the web?

No

Any other information, logs, or outputs that you want to share?

No

@amberdixon
Copy link
Author

Also note what happens when I comment out these lines from .bazelrc to disable sandboxing:

build --genrule_strategy=standalone
build --spawn_strategy=standalone

In this case, the absolute path of the HeaderB.h source file appears in the dot-d output:

~/Development/rules_ios amber/repro-inclusion-issue-bazel5 * tail -2  bazel-out/applebin_macos-darwin_x86_64-dbg-ST-45eefeec3d2c/bin/tests/ios/InclusionProblemRepro/_objs/LibraryA_objc/arc/D.d 
  tests/ios/InclusionProblemRepro/Classes/HeaderC.h \
  /Users/amberdixon/Development/rules_ios/tests/ios/InclusionProblemRepro/Classes/HeaderB.h

@amberdixon
Copy link
Author

amberdixon commented Nov 30, 2021

It looks like bazel5 added inclusion validation for objective-c with this commit: 38ab3cf

This issue is being exhibited with the objc source file is compiled with the -fmodules flag.

@amberdixon amberdixon changed the title inclusion validation fails when dot-d files have absolute paths inclusion validation fails when dot-d files have absolute paths for transitive includes Nov 30, 2021
@amberdixon amberdixon changed the title inclusion validation fails when dot-d files have absolute paths for transitive includes inclusion validation fails when dot-d files have absolute paths for transitive #includes Nov 30, 2021
@amberdixon
Copy link
Author

Check out my latest commit to my repro branch bazel-ios/rules_ios@ad1037a

It looks like this problem happens when all the following are true (1) we are compiling objc source with clang, (2) modules are enabled (i.e. -fmodules flag is on) and (3) we are using private header maps.

@amberdixon
Copy link
Author

@googlewalt Hi there! In this commit (38ab3cf) you said you "verified that the absolute paths are no longer showing up" in the dot-d files generated by clang. However, I have identified a scenario where absolute paths do show up. This happens if your source files have transitive includes (i.e. D includes C which includes B) and you are building with modules (i.e. -fmodules flag is on) and using header maps for the -iquote option.

Could you consider reverting this change? I think it would probably make sense to disable inclusion validation if enable_modules is true.

@jerrymarino
Copy link
Contributor

➕ this is definitely an issue when clang sticks absolute paths in the .d files. I'm pretty sure other rule sets in the community have this problem that use-fmodules and other circumstances: e.g. even with some vfsoverlay files it will make .d files absolute.

Perhaps there's a couple ways to handle this:

  1. allow absolute paths in .d files that are rooted in the bazel-out directory. this is effectively the same validation but is more robust
  2. If # 1 doesn't work, perhaps we could add a feature flag to --disable_cpp_header_validation. Perhaps this is orthogonal to # 1 - and perhaps a incompatible feature depending on if # 1 is possible

@amberdixon
Copy link
Author

@Wyverald hi there! This should be a bazel5 release blocker. The inclusion validation check is broken for this use case and one other use case as well, which I will add soon. Could you accept my PR or add a feature flag to disable inclusion validation in bazel5? Thanks!

@googlewalt
Copy link
Contributor

I looked into this a little bit. There seems to be multiple issues here.

  1. It seems the main reason for the breakage is due to sandboxing. Did we enable that recently?

  2. -fmodules does make things worse, such that even when we turn off sandboxing, inclusion check fails.

In the cross product of cases:

  1. Sandboxing off, no -fmodules (not needed to use header maps):

Test builds. The header absolute paths are relative to execRoot and HeaderDiscovery properly relativizes them.

  1. Sandboxing off, -fmodules

The following header has an absolute path that is not relative to execRoot, and inclusion check fails:
'/Users/googlewalt/rules_ios/tests/ios/InclusionProblemRepro/Classes/HeaderB.h'

  1. Sandboxing on, no -fmodules

Headers failing inclusion checks are all relative to a sandboxed path:

  '/private/var/tmp/_bazel_waltl/d12f6fec160b148428701d1ca0761002/sandbox/darwin-sandbox/13/execroot/build_bazel_rules_ios/tests/ios/InclusionProblemRepro/Classes/Extensions/D.m'
  '/private/var/tmp/_bazel_waltl/d12f6fec160b148428701d1ca0761002/sandbox/darwin-sandbox/13/execroot/build_bazel_rules_ios/rules/library/common.pch'
  '/private/var/tmp/_bazel_waltl/d12f6fec160b148428701d1ca0761002/sandbox/darwin-sandbox/13/execroot/build_bazel_rules_ios/tests/ios/InclusionProblemRepro/Classes/Extensions/D.h'
  '/private/var/tmp/_bazel_waltl/d12f6fec160b148428701d1ca0761002/sandbox/darwin-sandbox/13/execroot/build_bazel_rules_ios/tests/ios/InclusionProblemRepro/Classes/HeaderC.h'
  '/private/var/tmp/_bazel_waltl/d12f6fec160b148428701d1ca0761002/sandbox/darwin-sandbox/13/execroot/build_bazel_rules_ios/tests/ios/InclusionProblemRepro/Classes/HeaderB.h'
  1. Sandboxing on, -fmodules

Same behavior as (3).

The fact that clang emits different paths for 1 vs 2 (but not 3 vs 4) is pretty frustrating. For bazel, I imagine we would want to fix inclusion check to work properly for sandboxing. I'm not sure about to do about (2).

@Wyverald Wyverald added this to the Bazel 5.0 Release Blockers milestone Dec 7, 2021
@googlewalt
Copy link
Contributor

Inclusion check in objc_library can be disabled with --noobjc_use_dotd_pruning. With sandboxing on, this does not affect the correctness of builds, and there is some cost to the speed of incremental builds. Is this a reasonable workaround so that this is not a release blocker?

@sventiffe sventiffe added team-Rules-CPP Issues for C++ rules untriaged labels Dec 8, 2021
@jerrymarino
Copy link
Contributor

jerrymarino commented Dec 8, 2021

Thanks for the suggestion @googlewalt ! I think --noobjc_use_dotd_pruning could work for smaller builds with in-frequently changing headers. However, it's not a great solution given a non-trivial amount of transitive headers that change often. In that case, it blows up incremental build times - better to keep on Bazel 4.

Probably we can stop gap this quickly by a feature flag. I also think this would be a good capability to leave in long term given the validation logic is hard to get right and bakes several assumptions

If # 1 doesn't work, perhaps we could add a feature flag to --disable_cpp_header_validation. Perhaps this is orthogonal to # 1 - and perhaps a incompatible feature depending on if # 1 is possible

@amberdixon
Copy link
Author

@googlewalt +1 to what Jerry said. I have instead pushed up this PR: #14394 that will add a feature flag to disable CPP header validation.

Given that we are seeing these problems with the header inclusion validation logic, could you please take this PR into consideration to address this release blocker? Thanks!

@googlewalt
Copy link
Contributor

There is a commit in flight that will revert to 4.2's behavior and keep this disabled for Objective-C.

amberdixon pushed a commit to amberdixon/bazel that referenced this issue Dec 16, 2021
IncludeValidation was disabled for ObjC for bazel versions up to 4.2.
We are getting reports that turning it on in 5.0 causes breakages due
to Apple (?) clang emitting absolute paths in .d files.

Fixes bazelbuild#14346.

PiperOrigin-RevId: 415251009
Wyverald pushed a commit that referenced this issue Dec 16, 2021
IncludeValidation was disabled for ObjC for bazel versions up to 4.2.
We are getting reports that turning it on in 5.0 causes breakages due
to Apple (?) clang emitting absolute paths in .d files.

Fixes #14346.

PiperOrigin-RevId: 415251009

Co-authored-by: waltl <[email protected]>
Bencodes pushed a commit to Bencodes/bazel that referenced this issue Jan 10, 2022
IncludeValidation was disabled for ObjC for bazel versions up to 4.2.
We are getting reports that turning it on in 5.0 causes breakages due
to Apple (?) clang emitting absolute paths in .d files.

Fixes bazelbuild#14346.

PiperOrigin-RevId: 415251009
@FuegoFro
Copy link

I believe I'm running into this with non-Objective-C. Is it possible this happens for any usage of Clang?

Specifically I'm seeing issues where things like protoc are failing the declared dependencies check when downloaded from the remote cache, and it does indeed seem to be downloading .d files that have absolute paths from the CI machine in them.

For a minimal repro, I created a fresh repository from the following patch file:

diff --git a/.bazelversion b/.bazelversion
new file mode 100644
index 0000000..831446c
--- /dev/null
+++ b/.bazelversion
@@ -0,0 +1 @@
+5.1.0
diff --git a/BUILD b/BUILD
new file mode 100644
index 0000000..20708cf
--- /dev/null
+++ b/BUILD
@@ -0,0 +1,4 @@
+cc_binary(
+    name = "example",
+    srcs = ["example.c"],
+)
diff --git a/WORKSPACE b/WORKSPACE
new file mode 100644
index 0000000..e69de29
diff --git a/example.c b/example.c
new file mode 100644
index 0000000..61f21f3
--- /dev/null
+++ b/example.c
@@ -0,0 +1,7 @@
+#include <stdio.h>
+
+int main(int ac, char* av[])
+{
+    printf("Hello, World!");
+    return 0;
+}

Then after a bazelisk build //:example I have:

bazel-bin/_objs/example/example.d
bazel-out/darwin-fastbuild/bin/_objs/example/example.o: example.c \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/stdio.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/_stdio.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/cdefs.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_symbol_aliasing.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_posix_availability.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/Availability.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/AvailabilityVersions.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/AvailabilityInternal.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/_types.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/machine/_types.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/i386/_types.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_pthread/_pthread_types.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_va_list.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/machine/types.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/i386/types.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_int8_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_int16_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_int32_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_int64_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_u_int8_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_u_int16_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_u_int32_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_u_int64_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_intptr_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_uintptr_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_size_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_null.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/stdio.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/_ctermid.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_off_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/sys/_types/_ssize_t.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/secure/_stdio.h \
  /Users/dannyweinberg/Applications/Xcode-13.2.1.13C100.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/secure/_common.h

As you can see, this has absolute paths to Xcode, but isn't Objective-C, and therefore is still subject to the declared dependency checking. It's harder to get a minimal repro for the actual "this rule is missing dependency declarations for the following files" failure (that seems to require having a remote cache set up), but I'm definitely seeing it on my full builds that do leverage a remote cache populated by CI.

I'm not totally sure what the right fix is here (the proposed flag to turn off the validation until it's in a more stable state seems valuable), but it's currently breaking our ability to use Bazel 5.x and keep remote caching turned on.

I'm also happy to make a new issue instead if that would be preferable!

@burnpanck
Copy link

I am also seing this issue in non-objC. I am using clang in an embedded scenario, no use of -fmodules. I spent already too much time figuring out whats going on so I really don't have the energy to bisect the exact command-line flags that cause clang to output absolute paths, but it seems clear that bazel needs to be able to deal with absolute paths in .d files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules untriaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants