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

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

Closed
wants to merge 1 commit into from

Conversation

konste
Copy link

@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
@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 oquenchil and meteorcloudy and removed request for lberki March 31, 2023 06:41
@lberki
Copy link
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
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
This fixes bazelbuild#14947.

Closes bazelbuild#17928.

PiperOrigin-RevId: 521446074
Change-Id: I4bc155f0245bc1933e86cd0b37762263437ed1fe
meteorcloudy pushed a commit that referenced this pull request Apr 4, 2023
…le. (#17957)

This fixes #14947.

Closes #17928.

PiperOrigin-RevId: 521446074
Change-Id: I4bc155f0245bc1933e86cd0b37762263437ed1fe

Co-authored-by: Konstantin Erman <kerman@tableau.com>
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
This fixes bazelbuild#14947.

Closes bazelbuild#17928.

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

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
Member

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

@kkpattern
Copy link
Contributor

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
Member

meteorcloudy commented Sep 6, 2023

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.

name = "parse_showincludes",

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

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.

@kkpattern
Copy link
Contributor

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
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C++ dependency pruning is broken for Windows/MSVC
7 participants