Skip to content

Enable C++ deps pruning on Windows when PARSE_SHOWINCLUDES is available.#17928

Closed
konste wants to merge 1 commit intobazelbuild:masterfrom
konste:konste/issue-14947
Closed

Enable C++ deps pruning on Windows when PARSE_SHOWINCLUDES is available.#17928
konste wants to merge 1 commit intobazelbuild:masterfrom
konste:konste/issue-14947

Conversation

@konste
Copy link
Copy Markdown

@konste konste commented Mar 30, 2023

This fixes #14947.

@konste konste requested a review from lberki as a code owner March 30, 2023 16:32
@konste konste mentioned this pull request Mar 30, 2023
Closed
@ShreeM01 ShreeM01 added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Mar 30, 2023
@lberki lberki requested review from meteorcloudy and oquenchil and removed request for lberki March 31, 2023 06:41
@lberki
Copy link
Copy Markdown
Contributor

lberki commented Mar 31, 2023

I'll bounce this to @oquenchil and @meteorcloudy since they were more closely involved. Let me know if y'all need me to take a look at this.

@meteorcloudy meteorcloudy added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 31, 2023
@konste
Copy link
Copy Markdown
Author

konste commented Apr 3, 2023

@oquenchil this simple PR is a major blocker for us and badly needs your attention. Could you please merge it?

@oquenchil oquenchil removed the awaiting-review PR is awaiting review from an assigned reviewer label Apr 3, 2023
@copybara-service copybara-service bot closed this in 788801a Apr 3, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 3, 2023
ShreeM01 pushed a commit to ShreeM01/bazel that referenced this pull request Apr 3, 2023
@ShreeM01
Enable C++ deps pruning on Windows when PARSE_SHOWINCLUDES is available.
3f3e69b 
This fixes bazelbuild#14947.

Closes bazelbuild#17928.

PiperOrigin-RevId: 521446074
Change-Id: I4bc155f0245bc1933e86cd0b37762263437ed1fe
@ShreeM01 ShreeM01 mentioned this pull request Apr 3, 2023
Merged
meteorcloudy pushed a commit that referenced this pull request Apr 4, 2023
@ShreeM01
Enable C++ deps pruning on Windows when PARSE_SHOWINCLUDES is availab…
d3b59ad 
…le. (#17957)

This fixes #14947.

Closes #17928.

PiperOrigin-RevId: 521446074
Change-Id: I4bc155f0245bc1933e86cd0b37762263437ed1fe

Co-authored-by: Konstantin Erman <kerman@tableau.com>
@fweikert fweikert mentioned this pull request Apr 12, 2023
Closed
5 tasks
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
@fweikert
Enable C++ deps pruning on Windows when PARSE_SHOWINCLUDES is available.
c75deb5 
This fixes bazelbuild#14947.

Closes bazelbuild#17928.

PiperOrigin-RevId: 521446074
Change-Id: I4bc155f0245bc1933e86cd0b37762263437ed1fe
@kkpattern
Copy link
Copy Markdown
Contributor

kkpattern commented Sep 6, 2023

Hi, is it possible to provide a mechanism to disable pruning on Windows manually?

The main problem is that the /showIncludes output parsing is unreliable on Windows. There are two main problems:

  1. MSVC outputs the absolute path of the header files. When remote execution is involved, it's tricky to convert the path back to the relative path.
  2. Because of encoding and localization, it's hard for bazel to match the prefix of the /showIncludes output. For example, in our environment, the encoding is cp936, not utf-8. It's not feasible for bazel to try all the encodings plus all the languages MSVC may output.

If bazel fails to parse the /showIncludes output correctly, then when the header files are changed, the target won't get re-compiled, causing an incorrect build. This happened to us after we upgraded from 5.4.1 to 6.3.2

We also couldn't remove the parse_showincludes feature because then bazel will ask for the dotD file, which MSVC won't produce.

I have considered the following options：

  1. update isGenerateDotdFile method, adding an additional feature to tell bazel that the current toolchain won't produce the dotD file. Then we can disable pruning by removing the parse_showincludes feature.
  2. Add a command line flag to disable pruning.

If one of the above solutions is feasible, or there is a better solution, we are willing to help prepare a PR.

Thanks.

@meteorcloudy
Copy link
Copy Markdown
Member

meteorcloudy commented Sep 6, 2023

I believe we can make option 1 work, /cc @buildbreaker2021 as the C++ rule owner.

@kkpattern
Copy link
Copy Markdown
Contributor

kkpattern commented Sep 6, 2023

I did some experiments:

  1. We can add --action_env=VSLANG=1033 to force MSVC to generate output in English. This way, Bazel can parse the paths correctly, no matter their encoding.
  2. For remote execution, if we find /showIncludes in the action command parameters, we convert the output paths to paths relative to the working directory in the remote runner. Then, pruning works perfectly.

Though we can workaround the issue without changing Bazel itself, I think it's important to prevent pruning from causing wrong builds without the user's knowledge.

Since Bazel can only understand UTF-8 encoding, I think we can try decoding the output with UTF-8. If it fails, warn the user that pruning will not work correctly. Then, the user can choose to either change the encoding or turn off the parse_showincludes feature.

Should I create a post on bazel-discuss and continue the discussion there?

@meteorcloudy
Copy link
Copy Markdown
Member

meteorcloudy commented Sep 6, 2023 •
edited
Loading

We can add --action_env=VSLANG=1033 to force MSVC to generate output in English.

Nice finding! I guess we can add this env var as a default to the parse_showincludes feature? Or even all MSVC actions.

bazel/tools/cpp/windows_cc_toolchain_config.bzl

Line 791 in b27ca73

name = "parse_showincludes",

For remote execution, I remember we have some trick to ignore the execroot prefix, not sure why that doesn't work.

bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java

Lines 167 to 180 in b27ca73

for (String prefix : SHOW_INCLUDES_PREFIXES) {
if (line.startsWith(prefix)) {
line = line.substring(prefix.length()).trim();
Matcher m = EXECROOT_BASE_HEADER_PATTERN.matcher(line);
if (m.matches()) {
// Prefix the matched header path with "..\". This way, external repo header paths are
// resolved to "<execroot>\..\<repo name>\<path>", and main repo file paths are
// resolved to "<execroot>\..\<main repo>\<path>", which is nicely normalized to
// "<execroot>\<path>".
line = "..\\" + m.group("headerPath");
}
dependencies.add(line);
prefixMatched = true;
break;

Should I create a post on bazel-discuss and continue the discussion there?

Sure, we recommend to use https://github.com/bazelbuild/bazel/discussions now.

@Zeratul-Aiur Zeratul-Aiur mentioned this pull request Sep 7, 2023
Closed
@kkpattern
Copy link
Copy Markdown
Contributor

kkpattern commented Sep 8, 2023

I have created a discussion here: #19451.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@meteorcloudy meteorcloudy meteorcloudy approved these changes

@oquenchil oquenchil oquenchil approved these changes

Assignees

No one assigned

Labels

team-Rules-CPP Issues for C++ rules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

C++ dependency pruning is broken for Windows/MSVC

7 participants

@konste @lberki @kkpattern @meteorcloudy @oquenchil @sgowroji @ShreeM01