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 check should tolerate source file absolute paths #14364

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amberdixon
Copy link

Sometimes clang emits absolute paths to source files in dot-d output. Fixes #14346

@jerrymarino
Copy link
Contributor

Thanks for fixing this, this is definitely a blocker for Bazel 5. Is there any way we can get a test case in to ensure modules pass validation E2E and don't regress in the future? I think the module tests are pretty slim in Bazel.

Also I think a lot of the iOS community using C++ / ObjC is using modules: PodToBUILD, rules_ios and other rule sets are often enabling them by default

@googlewalt
Copy link
Contributor

See my comment in #14346.

@oquenchil oquenchil self-assigned this Dec 7, 2021
@oquenchil oquenchil added the team-Rules-CPP Issues for C++ rules label Dec 7, 2021
@Wyverald
Copy link
Member

Wyverald commented Dec 7, 2021

Thanks for fixing this, this is definitely a blocker for Bazel 5.

To confirm, why is this a blocker for 5.0? Is this behavior not present in Bazel 4.x?

@Wyverald
Copy link
Member

Wyverald commented Dec 7, 2021

Ah, sorry, I just saw #14346 (comment). It seems that it's actually a release blocker then.

@amberdixon
Copy link
Author

Hi! There is another situation where we run into problems with inclusion validation. I think it would be better for you to review this other PR instead which adds a feature flag to disable inclusion validation entirely: #14394 Thanks!

@amberdixon
Copy link
Author

Please consider #14394 instead. Thanks!!

@amberdixon amberdixon closed this Dec 8, 2021
@amberdixon
Copy link
Author

amberdixon commented Dec 8, 2021

Either this PR or the other one to disable inclusion validation (#14394) will unblock us.

However we strongly prefer that you merge the other PR (which is #14394), because the existing inclusion validation logic will likely create problems for other developers.

@amberdixon amberdixon reopened this Dec 8, 2021
@amberdixon
Copy link
Author

You can take this change under consideration in the future, if you decide to reenable the inclusion validation check for objective-c.

@amberdixon amberdixon closed this Dec 16, 2021
@amberdixon
Copy link
Author

Going to reopen this PR, because it correctly addresses the problems we had with the inclusion validation logic (which has been disabled for objective-c in bazel5-rc3 for the time being.)

@amberdixon amberdixon reopened this Dec 17, 2021
@oquenchil oquenchil removed their request for review February 16, 2022 14:48
@oquenchil oquenchil added team-Rules-ObjC Issues for Objective-C maintainers and removed team-Rules-CPP Issues for C++ rules labels Feb 16, 2022
@oquenchil oquenchil assigned googlewalt and unassigned oquenchil Feb 16, 2022
@sgowroji sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label May 4, 2022
@burnpanck
Copy link

burnpanck commented Nov 5, 2023

Would be great to get an actual solution for #14346 that doesn't just disable the check for obj-C. We're currently blocked by this and frantically trying to find a workaround, but so far unsuccessful. We experience this issue with C++ using clang, not even with -fmodules.

@protos-gunzinger
Copy link

protos-gunzinger commented Apr 24, 2024

Hi @burnpanck , thank you for your investigation;
I am currently hitting the same roadblock when trying to use a custom toolchain with clang (for embedded ARM: https://github.com/ARM-software/LLVM-embedded-toolchain-for-Arm ).
Did you find a workaround/solution for handling the absolute paths clangs outputs into the *.d files?

@burnpanck
Copy link

burnpanck commented Apr 24, 2024

Nice - I am also using that embedded ARM toolchain, and it was in that context where I first encountered this issue. My current workaround is a pretty sad one: A custom built bazel using a single change, here is the diff:

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 5dfb93b07d..46de5bc212 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -287,7 +287,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
     this.executionInfo = executionInfo;
     this.actionName = actionName;
     this.featureConfiguration = featureConfiguration;
-    this.needsIncludeValidation = cppSemantics.needsIncludeValidation();
+    this.needsIncludeValidation = false; // cppSemantics.needsIncludeValidation();
     this.builtInIncludeDirectories = builtInIncludeDirectories;
     this.additionalInputs = null;
     this.usedModules = null;

You see, this is just a hacky way of completely disabling inclusion validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer cla: yes team-Rules-ObjC Issues for Objective-C maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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