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

Absolute paths embedded in dependency files #11817

Open
thii opened this issue Jul 22, 2020 · 9 comments
Open

Absolute paths embedded in dependency files #11817

thii opened this issue Jul 22, 2020 · 9 comments
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug

Comments

@thii
Copy link
Member

thii commented Jul 22, 2020

Description of the problem / feature request:

When compiling objc_library targets, we're seeing absolute paths of generated files being embedded in dependency files (.d files).

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

We're trying to make the dependency files cacheable by removing or normalizing the absolute paths that are embedded in the dependency files.

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

Please check the attached project: nondeterministic-depfile.zip.

  • Run bazel build //:B
  • Check the B.d file contents. It contains an absolute path to a file that is generated during the build ( /private/var/tmp/_bazel_user/b4422045cf7215be2d3fa4e439c7376a/sandbox/darwin-sandbox/214/execroot/__main__/bazel-out/apl-darwin_x86_64-fastbuild/bin/AModule-module.modulemap)

The attached project uses some custom rules but the issue can be reproduced with an objc_library (that has modules enabled) depending on a swift_library and importing it via a Clang module.

What operating system are you running Bazel on?

macOS Catalina 10.15.5 (19F101)

What's the output of bazel info release?

release 3.4.1

Have you found anything relevant by searching the web?

Found a related issue with system headers: #7772. We found an additional issue with generated files during a build.

Found two issues with dependency files: #7769, #7771. The dependency_file feature can't be disabled because it's necessary to keep the C++ builds correct.

thii added a commit to thii/bazel that referenced this issue Jul 22, 2020
The dependency file generated by Clang can sometimes contain absolute
paths to system headers, system modules and files generated by Bazel
during the build.  These paths can defer between machines and between
Bazel instances, for example:

```
bazel-out/apl-darwin_x86_64-fastbuild/bin/_objs/B/arc/B.o: B.m \
  /private/var/tmp/_bazel_user/b4422045cf7215be2d3fa4e439c7376a/sandbox/darwin-sandbox/214/execroot/__main__/bazel-out/apl-darwin_x86_64-fastbuild/bin/A.modulemap \
  /Applications/Xcode-11.6.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Foundation.framework/Modules/module.modulemap
```

This patch adds a post-process to wrapped_clang to normalize those paths
by:

* converting absolute paths to relative paths for paths to files under
the execution root
* standardizing the paths to system headers, system modules to always
use the standard location of Xcode (`/Applications/Xcode.app`).  The
user doesn't need to have Xcode installed to this location as Bazel are
already mapping them to the correct location locally.

This makes the produced `.d` files deterministic and can reliably be
shared with the remote cache.

Fixes bazelbuild#11817.
thii added a commit to thii/bazel that referenced this issue Jul 22, 2020
The dependency file generated by Clang can sometimes contain absolute
paths to system headers, system modules and files generated by Bazel
during the build.  These paths can defer between machines and between
Bazel instances, for example:

```
bazel-out/apl-darwin_x86_64-fastbuild/bin/_objs/B/arc/B.o: B.m \
  /private/var/tmp/_bazel_user/b4422045cf7215be2d3fa4e439c7376a/sandbox/darwin-sandbox/214/execroot/__main__/bazel-out/apl-darwin_x86_64-fastbuild/bin/A.modulemap \
  /Applications/Xcode-11.6.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Foundation.framework/Modules/module.modulemap
```

This patch adds a post-process to wrapped_clang to normalize those paths
by:

* converting absolute paths to relative paths for paths to files under
the execution root
* standardizing the paths to system headers, system modules to always
use the standard location of Xcode (`/Applications/Xcode.app`).  The
user doesn't need to have Xcode installed to this location as Bazel are
already mapping them to the correct location locally.

This makes the produced `.d` files deterministic and can reliably be
shared with the remote cache.

Fixes bazelbuild#11817.
thii added a commit to thii/bazel that referenced this issue Jul 22, 2020
The dependency file generated by Clang can sometimes contain absolute
paths to system headers, system modules and files generated by Bazel
during the build.  These paths can defer between machines and between
Bazel instances, for example:

```
bazel-out/apl-darwin_x86_64-fastbuild/bin/_objs/B/arc/B.o: B.m \
  /private/var/tmp/_bazel_user/b4422045cf7215be2d3fa4e439c7376a/sandbox/darwin-sandbox/214/execroot/__main__/bazel-out/apl-darwin_x86_64-fastbuild/bin/A.modulemap \
  /Applications/Xcode-11.6.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Foundation.framework/Modules/module.modulemap
```

This patch adds a post-process to wrapped_clang to normalize those paths
by:

* converting absolute paths to relative paths for paths to files under
the execution root
* standardizing the paths to system headers, system modules to always
use the standard location of Xcode (`/Applications/Xcode.app`).  The
user doesn't need to have Xcode installed to this location as Bazel are
already mapping them to the correct location locally.

This makes the produced `.d` files deterministic and can reliably be
shared with the remote cache.

Fixes bazelbuild#11817.
@keith
Copy link
Member

keith commented Jul 22, 2020

Does using --experimental_inmemory_dotd_files work around this?

@thii
Copy link
Member Author

thii commented Jul 22, 2020

Tried but it didn’t change anything—the produced .d files are still part of the outputs and are still being cached as is.

From the documentation of the option, it looks like it’s just for the Remote Builds w/o the Bytes feature.

"If enabled, C++ .d files will be passed through in memory directly from the remote "
+ "build nodes instead of being written to disk."

@oquenchil oquenchil added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) labels Jul 28, 2020
@trybka
Copy link
Contributor

trybka commented Oct 26, 2020

I think non-determinism in .d files is harmless?
Is this leading to any observed action invalidation? My understanding is that .d files are parsed by bazel but not consumed by any downstream actions, and therefore don't affect any other cache keys.

@thii
Copy link
Member Author

thii commented Oct 26, 2020

Would it be fine to remove .d files from cache?

@thii thii closed this as completed Nov 19, 2020
@kkpattern
Copy link
Contributor

This actually makes us unable to use remote cache when building Android app. The c++ toolchain used to build Android app has cxx_builtin_include_directories point to directory in an external workspace(androidndk). Those builtin include files will have abosulte path in dotd files. When we hit remote cache, the build will failed with message:

this rule is missing dependency declarations for the following files included by 'engine/common/align16.cpp':
  'G:/bazel/execroot/__main__/external/androidndk/ndk/toolchains/llvm/prebuilt/windows-x86_64/lib64/clang/5.0.300080/include/stddef.h'

Since devs usually have different bazel execution root paths, other devs will failed to build the app. We tried -fdebug-prefix-map and it won't affect the dotd file. Seems the only way to solve the problem is to postprocess the dotd file, remove the execution root from the paths.

@kkpattern
Copy link
Contributor

This actually makes us unable to use remote cache when building Android app. The c++ toolchain used to build Android app has cxx_builtin_include_directories point to directory in an external workspace(androidndk). Those builtin include files will have abosulte path in dotd files. When we hit remote cache, the build will failed with message:

this rule is missing dependency declarations for the following files included by 'engine/common/align16.cpp':
  'G:/bazel/execroot/__main__/external/androidndk/ndk/toolchains/llvm/prebuilt/windows-x86_64/lib64/clang/5.0.300080/include/stddef.h'

Since devs usually have different bazel execution root paths, other devs will failed to build the app. We tried -fdebug-prefix-map and it won't affect the dotd file. Seems the only way to solve the problem is to postprocess the dotd file, remove the execution root from the paths.

After some experiments we found that it's caused by ndk clang on Windows "incorrectly" write absolute path for headers in gcc toolchain. The header files from clang are written with relative paths. On MacOS, the headers from gcc toolchain also are relative paths. But on Windows they're absolute paths. We don't know if this is a bug of ndk clang. We can fix this by adding -isystem external/androidndk/ndk/toolchains/llvm/prebuilt/windows-x86_64/lib64/clang/5.0.300080/include/ to the compile flags. Note that this directory need to be at the last of all builtin include directories. Since some headers in clang use #include_next to patch headers from gcc toolchain. Put the external/androidndk/ndk/toolchains/llvm/prebuilt/windows-x86_64/lib64/clang/5.0.300080/include/ before clang built include directories will cause compile error.

@keith
Copy link
Member

keith commented Sep 14, 2022

Ok I found another actual issue that this causes, that I believe relative .d paths would solve. On macOS in the case that the user downloads Xcode to one, potentially weird, location that is not included in

include_dirs = [
"/Applications/",
"/Library/",
]
user = repository_ctx.os.environ.get("USER")
if user:
include_dirs.append("/Users/{}/Library/".format(user))
# Include extra Xcode paths in case they're installed on other volumes
for toolchain in xcode_toolchains:
include_dirs.append(escape_string(toolchain.developer_dir))
return include_dirs

Then they build some C*, and then move Xcode to /Applications (where it ideally should have been all along), and build again, their local build artifacts, and local disk cache, are now polluted with the wrong absolute paths in the .d files (I assume the in memory .d files have the same issue), and they have to bazel clean --expunge, and entirely wipe their disk cache to get out of the cycle of undeclared inclusion errors. Steps

  1. Download Xcode, unzip in ~/Downloads
  2. Run a build using the Xcode.app in ~/Downloads
  3. Move Xcode.app to /Applications
  4. Run a build
  5. Get undeclared inclusion errors

@thii thii reopened this Oct 10, 2022
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Dec 15, 2023
@thii thii removed the stale Issues or PRs that are stale (no activity for 30 days) label Dec 26, 2023
@meteorcloudy
Copy link
Member

/cc @ahumesky

@meteorcloudy meteorcloudy added not stale Issues or PRs that are inactive but not considered stale team-Android Issues for Android team labels Jul 30, 2024
copybara-service bot pushed a commit that referenced this issue Aug 2, 2024
…ache issues with the Android NDK on Windows. See #11817

PiperOrigin-RevId: 658725449
Change-Id: I09edfa9af8a4956d661174bef76a7f8776891a8b
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Aug 19, 2024
…ache issues with the Android NDK on Windows. See bazelbuild#11817

PiperOrigin-RevId: 658725449
Change-Id: I09edfa9af8a4956d661174bef76a7f8776891a8b
@ahumesky ahumesky removed the team-Android Issues for Android team label Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants