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

Make filetypes case insensitive on Windows #14005

Closed

Conversation

torgil
Copy link
Contributor

@torgil torgil commented Sep 17, 2021

Filetypes are based on checking file-extensions which are case insensitive on Windows.
For example, A lib file could be named .lib, Lib, etc which is currently not allowed
in for example cc_import.

This patch allows file extensions to vary in case on Windows.

@torgil
Copy link
Contributor Author

torgil commented Sep 17, 2021

This is aimed to solve issues like #13879 (comment)

Is it better to add the case insensitivity case-by-case (or even casing case-by-case) rather than going for a general approach?

@oquenchil oquenchil removed the request for review from lberki September 27, 2021 11:28
@oquenchil oquenchil added area-Windows Windows-specific issues and feature requests team-Rules-CPP Issues for C++ rules labels Sep 27, 2021
@torgil torgil force-pushed the case-insensitive-windows-filetypes branch from 4bfa043 to a7f9d6c Compare October 5, 2021 13:53
Filetypes are based on checking file-extensions which are case insensitive on Windows.
For example, A lib file could be named .lib, Lib, etc which is currently not allowed
in for example cc_import.

This patch allows file extensions to vary in case on Windows.
@torgil torgil force-pushed the case-insensitive-windows-filetypes branch from a7f9d6c to d1f92e4 Compare October 6, 2021 07:34
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bazel-io bazel-io closed this in 861584c Oct 7, 2021
@@ -96,7 +98,7 @@ public boolean apply(String path) {

@Override
public boolean apply(String path) {
return (path.endsWith(ext) && !PIC_ASSEMBLER.matches(path)) || path.endsWith(".asm");
return (OS.endsWith(path, ext) && !PIC_ASSEMBLER.matches(path)) || OS.endsWith(path, ".asm");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could make .S files end up with ASSEMBLE action instead of PREPROCESS_ASSEMBLE on Windows?

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java:

} else if (CppFileTypes.ASSEMBLER.matches(sourcePath)) {
  return CppActionNames.ASSEMBLE;
} else if (CppFileTypes.ASSEMBLER_WITH_C_PREPROCESSOR.matches(sourcePath)) {
  return CppActionNames.PREPROCESS_ASSEMBLE;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, is this a serious problem? Do you know how to distinguish assemble and preprocess assemble actions on Windows?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meteorcloudy we just saw that this is a problem also with .c and .C files, on one side Bazel considers .c as C and .C as C++, once you are case insensitive on Windows there is the problem that the .c files end up as C++ files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks like Make filetypes case insensitive is going to be an incompatible change, which may not have much gains for us. I think maybe the correctly way to is to make filetype configuration first? Just like the artifcat extension for binary/static library/dynamic library /cc @oquenchil

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this is the issue #15073

Gormo pushed a commit to Gormo/bazel that referenced this pull request Oct 19, 2021
This fixes an issue introduced by PR bazelbuild#14005 where .s and .S
extensions were handled case-insensitive on Windows so the action
assemble was triggered instead of preprocess_assemble.
Gormo pushed a commit to Gormo/bazel that referenced this pull request Nov 9, 2021
This fixes an issue introduced by PR bazelbuild#14005 where .s and .S
extensions were handled case-insensitive on Windows so the action
assemble was triggered instead of preprocess_assemble.
Gormo pushed a commit to Gormo/bazel that referenced this pull request Nov 9, 2021
This fixes an issue introduced by PR bazelbuild#14005 where .s and .S
extensions were handled case-insensitive on Windows so the action
assemble was triggered instead of preprocess_assemble.
bazel-io pushed a commit that referenced this pull request Nov 24, 2021
This fixes an issue introduced by PR #14005 where .s and .S
extensions were handled case-insensitive on Windows so the action
assemble was triggered instead of preprocess_assemble.

Closes #14131.

PiperOrigin-RevId: 412005097
Bencodes pushed a commit to Bencodes/bazel that referenced this pull request Jan 10, 2022
This fixes an issue introduced by PR bazelbuild#14005 where .s and .S
extensions were handled case-insensitive on Windows so the action
assemble was triggered instead of preprocess_assemble.

Closes bazelbuild#14131.

PiperOrigin-RevId: 412005097
fmeum pushed a commit to fmeum/bazel that referenced this pull request Jul 27, 2022
This fixes an issue introduced by PR bazelbuild#14005 where .s and .S
extensions were handled case-insensitive on Windows so the action
assemble was triggered instead of preprocess_assemble.

Closes bazelbuild#14131.

PiperOrigin-RevId: 412005097
sgowroji pushed a commit to sgowroji/bazel that referenced this pull request Jul 27, 2022
This fixes an issue introduced by PR bazelbuild#14005 where .s and .S
extensions were handled case-insensitive on Windows so the action
assemble was triggered instead of preprocess_assemble.

Closes bazelbuild#14131.

PiperOrigin-RevId: 412005097
sgowroji pushed a commit that referenced this pull request Jul 27, 2022
This fixes an issue introduced by PR #14005 where .s and .S
extensions were handled case-insensitive on Windows so the action
assemble was triggered instead of preprocess_assemble.

Closes #14131.

PiperOrigin-RevId: 412005097

Co-authored-by: Simon Bjorklen <[email protected]>
kkpattern added a commit to kkpattern/bazel that referenced this pull request 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 .
copybara-service bot pushed a commit that referenced this pull request Apr 21, 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
fweikert pushed a commit to fweikert/bazel that referenced this pull request 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 pull request 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
iancha1992 pushed a commit that referenced this pull request 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Windows Windows-specific issues and feature requests cla: yes team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants