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

cc_libaray target won't be rebuilt when header files are modified #19439

Closed
dhmemi opened this issue Sep 7, 2023 · 18 comments
Closed

cc_libaray target won't be rebuilt when header files are modified #19439

dhmemi opened this issue Sep 7, 2023 · 18 comments
Assignees
Labels

Comments

@dhmemi
Copy link
Contributor

dhmemi commented Sep 7, 2023

Description of the bug:

cc_libaray target won't be rebuilt when header files list in hdrs attr are modified.

Which category does this issue belong to?

C++/Objective-C Rules

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

Create a target with a c++ header file and a c++ source file:

cc_library(
    name = "my_lib",
    hdrs = ["my_lib.hpp"],
)

cc_binary(
    name = "main",
    srcs = ["main.cpp"],
    deps = ["my_lib"]
)

my_lib.hpp content:

#include <string>

struct MyLib {
  std::string value = "my_lib";
};

main.cpp content:

#include <iostream>
#include "my_lib.hpp"

int main(){
  MyLib obj;
  std::cout << obj.value << std::endl;
  return 0;
}

Run bazel run my_lib. and modify my_lib.hpp to set default value of MyLib::value to another string (do not modify the main.cpp), run bazel run my_lib again. We will get a same output as
the target will not be rebuilt.

Which operating system are you running Bazel on?

Windows

What is the output of bazel info release?

6.3.2

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

No response

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

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

No response

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

Note that I test all release version from 6.1.1 to 6.3.2, This bug exists in version 6.2.0 and later versions, but 6.1.1 ~ 6.1.2 works well.

@fmeum
Copy link
Collaborator

fmeum commented Sep 7, 2023

cc_library targets can't be run with bazel run and cc_binary targets do not have a hdrs attribute. Could you fix the reproducer?

@dhmemi
Copy link
Contributor Author

dhmemi commented Sep 7, 2023

cc_library targets can't be run with bazel run and cc_binary targets do not have a hdrs attribute. Could you fix the reproducer?

fixed.

@Zeratul-Aiur
Copy link

Zeratul-Aiur commented Sep 7, 2023

See:#17928 (comment)

@fmeum
Copy link
Collaborator

fmeum commented Sep 11, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Sep 11, 2023
@iancha1992
Copy link
Member

@bazel-io fork 6.4.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Sep 11, 2023
@iancha1992
Copy link
Member

@dhmemi Are you still pushing this to be part of 6.4.0? The expected first release candidate date is next Monday (9/18/2023), just FYI. Thanks!

@dhmemi
Copy link
Contributor Author

dhmemi commented Sep 12, 2023

@dhmemi Are you still pushing this to be part of 6.4.0? The expected first release candidate date is next Monday (9/18/2023), just FYI. Thanks!

No, According to #19451, I solved the problem temporarily by installing English language package in VS and adding the environment variable VSLANG.

@fmeum
Copy link
Collaborator

fmeum commented Sep 12, 2023

@dhmemi Are you still pushing this to be part of 6.4.0? The expected first release candidate date is next Monday (9/18/2023), just FYI. Thanks!

We absolutely should as this is a correctness issue. If we don't fix it in time, we should roll back #17928 instead.

CC @meteorcloudy

@meteorcloudy
Copy link
Member

If we set VSLANG and the user doesn't have English language pack installed, will the build fail?

@fmeum
Copy link
Collaborator

fmeum commented Sep 12, 2023

If we set VSLANG and the user doesn't have English language pack installed, will the build fail?

No, it will silently fall back to an available locale. I am working on a fix.

@meteorcloudy
Copy link
Member

Thanks!

bazel-io pushed a commit to bazel-io/bazel that referenced this issue Sep 14, 2023
The auto-configured Windows toolchain now only enables `/showIncludes` if the English language pack for VS is installed and the compiler language can thus be forced to be English. Other languages may use encodings other than ASCII or UTF-8 and thus fail to be recognized by Bazel's `ShowInputFilter`. A warning with a suggested fix is shown if this fails.

See bazelbuild#19451
Fixes bazelbuild#19439

RELNOTES: Compilation actions using the auto-configured MSVC toolchain are forced to emit error messages in English if the English language pack for Visual Studio is installed.

Closes bazelbuild#19497.

PiperOrigin-RevId: 565251294
Change-Id: I926c484c21310ee4d7b5e8d5756cb61e45d6e6ac
meteorcloudy pushed a commit that referenced this issue Sep 14, 2023
The auto-configured Windows toolchain now only enables `/showIncludes`
if the English language pack for VS is installed and the compiler
language can thus be forced to be English. Other languages may use
encodings other than ASCII or UTF-8 and thus fail to be recognized by
Bazel's `ShowInputFilter`. A warning with a suggested fix is shown if
this fails.

See #19451
Fixes #19439

RELNOTES: Compilation actions using the auto-configured MSVC toolchain
are forced to emit error messages in English if the English language
pack for Visual Studio is installed.

Closes #19497.

Commit
0f10359

PiperOrigin-RevId: 565251294
Change-Id: I926c484c21310ee4d7b5e8d5756cb61e45d6e6ac

Co-authored-by: Fabian Meumertzheim <[email protected]>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks!

@dhmemi
Copy link
Contributor Author

dhmemi commented Sep 21, 2023

I test 6.4.0 RC1, It works well even if build without flag --action_env=VSLANG=1033 if the Visual Studio English language pack is installed.

But, If the English language pack isn't installed, no warnings or errors output, and the build result is still incorrect, the output from the first build will be like:

PS C:\Users\dhmem\Documents\codes\test-baz> bazelisk run main
INFO: Analyzed target //:main (36 packages loaded, 178 targets configured).
INFO: Found 1 target...
INFO: From Compiling my_lib.cpp:
注意: 包含文件:  C:\users\dhmem\_bazel_dhmem\uz4x2ev4\execroot\__main__\my_lib.hpp
INFO: From Compiling main.cpp:
注意: 包含文件:  C:\Program Files (x86)\Windows Kits\10\include\10.0.22000.0\ucrt\stdio.h
注意: 包含文件:   C:\Program Files (x86)\Windows Kits\10\include\10.0.22000.0\ucrt\corecrt.h
注意: 包含文件:    C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\include\vcruntime.h
注意: 包含文件:     C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\include\sal.h
注意: 包含文件:      C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\include\concurrencysal.h
注意: 包含文件:     C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\include\vadefs.h
注意: 包含文件:   C:\Program Files (x86)\Windows Kits\10\include\10.0.22000.0\ucrt\corecrt_wstdio.h
注意: 包含文件:    C:\Program Files (x86)\Windows Kits\10\include\10.0.22000.0\ucrt\corecrt_stdio_config.h
注意: 包含文件:  C:\users\dhmem\_bazel_dhmem\uz4x2ev4\execroot\__main__\my_lib.hpp
Target //:main up-to-date:
  bazel-bin/main.exe
INFO: Elapsed time: 0.799s, Critical Path: 0.52s
INFO: 9 processes: 5 internal, 4 local.
INFO: Build completed successfully, 9 total actions
INFO: Running command line: bazel-bin/main.exe
Object value: 80
PS C:\Users\dhmem\Documents\codes\test-baz>

And if I change my_lib.h and build again, nothing will be rebuild.

@meteorcloudy
Copy link
Member

@dhmemi after you uninstall the english pack, did you re-fetch the cc toolchain? You can do that by bazel clean --expunge first.

@fmeum
Copy link
Collaborator

fmeum commented Sep 21, 2023

@meteorcloudy What do you think of showing a warning telling users to run bazel sync --configure or bazel clean --expunge if an MSVC action emits stderr output matching something like ...: ...: ...\execroot\... that isn't recognized by the ShowIncludesFilter?

@meteorcloudy
Copy link
Member

meteorcloudy commented Sep 21, 2023

SGTM! Or we can just ask users to install the English language pack?

@fmeum
Copy link
Collaborator

fmeum commented Sep 21, 2023

SGTM! Or we can just ask users to install the English language pack?

Even after they install it, they will still need to run these commands. But yes, we should include in the message as well.

It is quite unfortunate that we need to tell users to run bazel clean --expunge though to cover Bzlmod.

@dhmemi
Copy link
Contributor Author

dhmemi commented Sep 21, 2023

@dhmemi after you uninstall the english pack, did you re-fetch the cc toolchain? You can do that by bazel clean --expunge first.

My omission, after run bazel clean --expunge, warning message prints correctly.

copybara-service bot pushed a commit that referenced this issue Sep 25, 2023
This error ensures that removing the English language pack for MSVC
without also refetching `@local_config_cc` doesn't result in silently
incorrect incremental builds.

I considered making this a warning instead, but that would either result
in warning spam (if a false positive) or drown in the unfiltered MSVC
output (if a true positive). Making this an error is better for
correctness and will lead users to report any false positive matches.

Fixes #19439 (comment)

Closes #19580.

PiperOrigin-RevId: 568133958
Change-Id: If43924da6ba2f332edf4db0aed24056aa89fbb75
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Sep 25, 2023
This error ensures that removing the English language pack for MSVC
without also refetching `@local_config_cc` doesn't result in silently
incorrect incremental builds.

I considered making this a warning instead, but that would either result
in warning spam (if a false positive) or drown in the unfiltered MSVC
output (if a true positive). Making this an error is better for
correctness and will lead users to report any false positive matches.

Fixes bazelbuild#19439 (comment)

Closes bazelbuild#19580.

PiperOrigin-RevId: 568133958
Change-Id: If43924da6ba2f332edf4db0aed24056aa89fbb75
meteorcloudy pushed a commit that referenced this issue Sep 25, 2023
This error ensures that removing the English language pack for MSVC
without also refetching `@local_config_cc` doesn't result in silently
incorrect incremental builds.

I considered making this a warning instead, but that would either result
in warning spam (if a false positive) or drown in the unfiltered MSVC
output (if a true positive). Making this an error is better for
correctness and will lead users to report any false positive matches.

Fixes
#19439 (comment)

Closes #19580.

Commit
a0497a0

PiperOrigin-RevId: 568133958
Change-Id: If43924da6ba2f332edf4db0aed24056aa89fbb75

Co-authored-by: Fabian Meumertzheim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
9 participants