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

--host_cxxopt and --cxxopt are passed to the compiler also for .c files on Windows #15073

Closed
foxandi opened this issue Mar 18, 2022 · 26 comments
Closed
Assignees
Labels
help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: bug

Comments

@foxandi
Copy link
Contributor

foxandi commented Mar 18, 2022

Description of the problem / feature request:

Since Bazel 5.0.0 --host_cxxopt and --cxxopt are passed to compilers compiling .c files on Windows.

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

Use the autoconfigure toolchain to build (with -s) a .c file in Windows. Set --host_cxxopt and --cxxopt in the .bazelrc and see in the compiler call the passed opts from the .bazelrc. Doing the same on Linux show, that the opts here are not passed.

What operating system are you running Bazel on?

Windows 10 Enterprise (Version 10.0.19042 Build 19042)

What's the output of bazel info release?

release 5.0.0

Have you found anything relevant by searching the web?

#14005
#14131

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

This commit 861584c introduced case insensity for files types on Windows

private static final OsPathPolicy OS = OsPathPolicy.getFilePathOs();

The change also leads to .c files under Windows match to C_SOURCE (.c extension) and CPP_SOURCE (.C extension)

public static final FileType CPP_SOURCE =
FileType.of(".cc", ".cpp", ".cxx", ".c++", ".C", ".cu", ".cl");

Which results in passing cxxopt for .c files

if (CppFileTypes.CPP_SOURCE.matches(sourceFilename)
|| CppFileTypes.CPP_HEADER.matches(sourceFilename)
|| CppFileTypes.CPP_MODULE_MAP.matches(sourceFilename)
|| CppFileTypes.CLIF_INPUT_PROTO.matches(sourceFilename)) {
flagsBuilder.addAll(config.getCxxopts());
}

@meteorcloudy
Copy link
Member

Maybe this is a similar bug that should be fixed like edfe2a1?

@meteorcloudy
Copy link
Member

/cc @buildbreaker2021

@oquenchil oquenchil added type: feature request P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged labels Mar 28, 2022
@kkpattern
Copy link
Contributor

We also encounter this problem. Unfortunately this causes us to not be able to compile Android on windows. Which is a major block for our project. Is there a rough estimate of when this will be fixed? Or is there any way we can workaround this? Thanks.

@oakad
Copy link

oakad commented Jul 15, 2022

The worst thing here is that this issue is not even particularly complicated, even with WindowsOsPathPolicy rework included (the right thing to do: support for case sensitive directories on Windows 10+ will solve a lot of problems). Yet, it breaks a whole lot of plain C libraries on Windows putting a big question mark on Bazel's viability in general.

Which is a pity.

@kkpattern
Copy link
Contributor

The worst thing here is that this issue is not even particularly complicated, even with WindowsOsPathPolicy rework included (the right thing to do: support for case sensitive directories on Windows 10+ will solve a lot of problems). Yet, it breaks a whole lot of plain C libraries on Windows putting a big question mark on Bazel's viability in general.

Which is a pity.

Currently, you can workaround this by replacing --cxxopt with --per_file_copt=.*\.cpp\$@.

But I agree that it's very confusing why this issue is a feature request and is P4.

@oakad
Copy link

oakad commented Jul 15, 2022

It won't work for flags which are set in the actual BUILD files. The primary issue here, is that bazel offers no way to say "this file is plain C" explicitly. So, in a project with several dozens of third party dependencies a lot of patching and testing is needed to get anywhere with Bazel v5.

Bazel v4 sort of works, but then it can not be used with VS2022 due to another open issue.

@kkpattern
Copy link
Contributor

Bazel v4 sort of works, but then it can not be used with VS2022 due to another open issue.

I thought this is not a problem for a msbuild based toolchain because msbuild only gives a warning? For us, it's a block issue because we're doing android cross compile on Windows and pass C++ flag to C compile simply failed.

Another interesting workaround is that we found somehow the C++ flags defined in toolchain won't be passed to C targets. So we end up moving the C++ flags to toolchain. But yeah it's lucky for us to be able to workaround this bug. For some complex project this may not work.

@oakad
Copy link

oakad commented Jul 15, 2022

VS2022 is broken pretty badly, we had to go back to VS2019/bazel v4 for now. We may figure something out going forward; possibly by making all sources C++ compatible (this appears to be the best option on the cost/benefit scale).

Still, not nice.

@meteorcloudy
Copy link
Member

@oquenchil This issue seems to have wide user impact on Windows. Is it caused by starlarkfifying the cc rules? Do you know which exact change caused this and how hard is it to fix this?

@kkpattern
Copy link
Contributor

@meteorcloudy Looks like is caused by #14005. The .c extension is matched with .C cause bazel treat it as a C++ source. I think this can be fixed like edfe2a1 . I understand bazel team has many things to do and may not have the capacity to fix this. If you have the bandwidth to review a PR I can give this a try.

@meteorcloudy
Copy link
Member

Not sure why .c and .C should mean different file types on Windows because file names are already case insensitive. Is this a Linux convention? Can you just change files with .C to .cpp?
But given we are already making an exception in edfe2a1, a PR to fix this sounds plausible, @oquenchil @comius should decide eventually.

@kkpattern
Copy link
Contributor

Can you just change files with .C to .cpp?

We don't actually have any .C files. I think it's bazel that treated all .c files as .C files causing the problem.

@oakad
Copy link

oakad commented Jul 18, 2022

@meteorcloudy Also see #15550 for some further info.

@Danvil
Copy link

Danvil commented Dec 1, 2022

Any progress on this issue? Bazel cannot compile C targets under Windows anymore since version 5. I don't think this warrants classification as a "feature request" or "P4".

@oakad
Copy link

oakad commented Dec 2, 2022

Bazel can compile things on Windows, it just requires jumping through ugly loops with --per_file_copt= and possibly some patching.

But yes, this issue is bad and the lack of proper support for case sensitive abilities of NTFS is also unfortunate - that is, bazel would have worked fine on Windows with some extra configs if not for the rather stupid case forcing inside the Windows path resolver.

@Danvil
Copy link

Danvil commented Dec 2, 2022

From the documentation: --cxxopts: This is similar to --copt, but only applies to C++ compilation, not to C compilation or linking. In obvious contradiction bazel 5.3.2 passes --cxxopts arguments to C files. This is clearly a bug and not a "feature request". @oquenchil

I find if concerning that something that elementary is broken in bazel and swiped under the rug.

@kalmard0
Copy link

This bug is still causing significant problems in a project, forcing us to use an old bazel version which causes further compatibility issues and surfaces other bugs without fixes. Will it ever be looked at?

@oakad
Copy link

oakad commented Apr 17, 2023

It's almost like Windows is not an important OS for developers any more. :-D

Yet, Windows is still the most used developer OS by a huge margin.

@meteorcloudy meteorcloudy added P2 We'll consider working on this in future. (Assignee optional) help wanted Someone outside the Bazel team could own this and removed P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) labels Apr 17, 2023
@meteorcloudy
Copy link
Member

@oquenchil I think this shouldn't be a P4 issue, can you take a look?

If anyone has a fix for this, a PR is also very welcome!

kkpattern added a commit to kkpattern/bazel that referenced this issue Apr 17, 2023
This fixes an issue introduced by PR bazelbuild#14005 where .c and .C
extensions were handled case-insensitive on Windows so the cxxopt will be passed to C source files.

Closes bazelbuild#15073 .
@kkpattern
Copy link
Contributor

Hi, I've created a PR. It's basically learned from edfe2a1 . I'm not very familiar to java actually, so please tell me if I did anything wrong. I've tested it on my environment and added a test.

fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
This fixes an issue introduced by PR bazelbuild#14005 where .c and .C extensions were handled case-insensitive on Windows so the cxxopt will be passed to C source files.

Closes bazelbuild#15073 .

Closes bazelbuild#18119.

PiperOrigin-RevId: 526001251
Change-Id: I464e5feae397bdac443ddd159309f77071629e01
FaBrand pushed a commit to FaBrand/bazel that referenced this issue Jun 1, 2023
This fixes an issue introduced by PR bazelbuild#14005 where .c and .C extensions were handled case-insensitive on Windows so the cxxopt will be passed to C source files.

Closes bazelbuild#15073 .

Closes bazelbuild#18119.

PiperOrigin-RevId: 526001251
Change-Id: I464e5feae397bdac443ddd159309f77071629e01
@FaBrand
Copy link
Contributor

FaBrand commented Jun 1, 2023

Created the referenced Cherrypick @kkpattern Any objections agains the attempt?

@kkpattern
Copy link
Contributor

Created the referenced Cherrypick @kkpattern Any objections agains the attempt?

Not at all😃.

iancha1992 pushed a commit that referenced this issue Jun 12, 2023
This fixes an issue introduced by PR #14005 where .c and .C extensions were handled case-insensitive on Windows so the cxxopt will be passed to C source files.

Closes #15073 .

Closes #18119.

PiperOrigin-RevId: 526001251
Change-Id: I464e5feae397bdac443ddd159309f77071629e01

Co-authored-by: Kai Zhang <kylerzhang11@gmail.com>
davidben added a commit to google/boringssl that referenced this issue Jul 5, 2023
This reverts commit 28e4a1b. Bazel
broke --cxxopt on Windows in
bazelbuild/bazel#15073, which means projects
enabling, say, C++20 with --cxxopt=/std:c++20 are silently passing
/std:c++20 to our C files.

This is already a problem, but MSVC is smart enough to silently ignore
the flag when building C. However, MSVC will report an error if you then
pass /std:c++20 /std:c11 into the same command. It seems that check is
not aware of this ignoring behavior.

Ultimately, this is a Bazel bug, and one that makes the broken versions
of Bazel unsuitable for use with C. This was fixed in Bazel in
bazelbuild/bazel#18119 and backported to the
upcoming Bazel 6.3.0 release in
bazelbuild/bazel#18552

Temporarily revert the change. When Bazel 6.3.0 is released, we'll put
this back and require Windows users use a functioning version of Bazel.

Bug: 624
Fixed: 623
Change-Id: I68d9b2ed8751b4cf5dc7f42f8c1fbd42a97d6ca2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61365
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
@aaron-michaux
Copy link

aaron-michaux commented Aug 18, 2023

Why is this closed? Observed in the latest 5.x and 6.x builds.

This is C code! sign.c is a C source file! --host_cxxopt=-std=c++17 causes -std=c++17 to be passed to C compilation!!!

/home/.../.cache/bazel/_bazel_build/install/a09dbb90c658248f08f9aa0eba11997d/process-wrapper '--timeout=0' '--kill_delay=15' '--stats=/home/.../.cache/bazel/_bazel_build/36f2a9693014a18078cf75940b43707b/sandbox/processwrapper-sandbox/114/stats.out' \

/usr/bin/gcc-9 -MD -MF bazel-out/k8-dbg-gcc/bin/external/boringssl/_objs/crypto/sign.pic.d \
   '-frandom-seed=bazel-out/k8-dbg-gcc/bin/external/boringssl/_objs/crypto/sign.pic.o' \
   '-DBAZEL_CURRENT_REPOSITORY="boringssl"' \
   -iquote external/boringssl -iquote bazel-out/k8-dbg-gcc/bin/external/boringssl \
   -isystem external/boringssl/src/include \
   -isystem bazel-out/k8-dbg-gcc/bin/external/boringssl/src/include \
   '-std=c++17' \ # WUHT?????
   -g -O0 -fno-omit-frame-pointer -fPIC -fno-canonical-system-headers \
   '-fdiagnostics-color=always' -Wa,--noexecstack -Wall -Werror '-Wformat=2' \
   -Wsign-compare -Wmissing-field-initializers -Wwrite-strings -Wshadow \
   -fno-common '-D_XOPEN_SOURCE=700' '-std=c11' -Wmissing-prototypes \
   -Wold-style-definition -Wstrict-prototypes -c external/boringssl/src/crypto/evp/sign.c \
   -o bazel-out/k8-dbg-gcc/bin/external/boringssl/_objs/crypto/sign.pic.o

@meteorcloudy
Copy link
Member

6.3.0 should contain the fix? #18552

@kkpattern
Copy link
Contributor

I did a simple test. When compile with MSVC with --cxxopt=/std:c++17, bazel 5.1.1 added the flag to a C source file:

C:\...\cl.exe ... -D__TIME__="redacted" /std:c++17 ... /c hello.c
                                         ^^^^^^^^^

while bazel 6.3.2 didn't:

C:\...\cl.exe ... -D__TIME__="redacted" ... /c hello.c

@aaron-michaux
Copy link

aaron-michaux commented Aug 18, 2023

I'm using Ubuntu 22.04... so I'm seeing the problem on Linux. Given that we're not using Bzlmod (because of incompatibilities), it's possible that I have a dependency of a dependency that's getting an old version of cc_tools or something. It's frustrating because it my build is broken because of two rather trivial bugs that look like they're in bazel. (Where does @local_config_cc come from, after all???)

davidben added a commit to google/boringssl that referenced this issue Sep 1, 2023
This reverts 1e2f169. Bazel 6.3 has
since been released, which includes a fix for
bazelbuild/bazel#15073. Envoy and gRPC have
both since updated to this Bazel version. The policies in
https://opensource.google/documentation/policies/cplusplus-support#build_systems
also imply a minimum Bazel version of 6.3.2.

I'm thinking we let this bake for a little while, to catch any
unexpected issues, and then, if it sticks, we try to go ahead and
require C11 across the board.

Update-Note: If using Bazel with MSVC, and the build fails with
something like "Command line error D8016 : '/std:c++20' and '/std:c11'
command-line options are incompatible", you are likely running into the
above Bazel bug. Update to Bazel 6.3 or later.

Bug: 623, 624
Change-Id: I8baa99392ca47bc7580bc2930e7f4b16beced91e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62905
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
copybara-service bot pushed a commit to tink-crypto/tink-cc that referenced this issue Jan 8, 2024
 * Move testonly deps to `tink_cc_testonly_deps()`
 * Upgrade deps:
   * googletest (=> 1.14)
   * absl (=> 20230802.1)
   * BoringSSL (=> commit 667d54c96acda029523c5bf425e8eb9079dbe94a from the `master-with-bazel` branch)
   * bazel_skylib (=> 1.5.0)
   * bazel (=> 6.4.0) (this is needed because of [1] and [2])

[1] bazelbuild/bazel#15073
[2] google/boringssl@235ee97

PiperOrigin-RevId: 596709495
Change-Id: I04e978161ee9b6adf8ccdf3e73f8a54abffc7ded
morambro added a commit to tink-crypto/tink that referenced this issue Jan 23, 2024
 * Move testonly deps to `tink_cc_testonly_deps()`
 * Upgrade deps:
   * googletest (=> 1.14)
   * absl (=> 20230802.1)
   * BoringSSL (=> commit 667d54c96acda029523c5bf425e8eb9079dbe94a from the `master-with-bazel` branch)
   * bazel_skylib (=> 1.5.0)
   * bazel (=> 6.4.0) (this is needed because of [1] and [2])

[1] bazelbuild/bazel#15073
[2] google/boringssl@235ee97

PiperOrigin-RevId: 596709495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants