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.

Sign up for GitHub

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

Jump to bottom

Make filetypes case insensitive on Windows #14005

Closed
wants to merge 2 commits into from
Closed

Make filetypes case insensitive on Windows #14005

wants to merge 2 commits into from

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.

@google-cla google-cla bot added the cla: yes label Sep 17, 2021
@torgil torgil requested a review from lberki as a code owner September 17, 2021 15:32
@torgil
Copy link
Contributor Author

torgil commented Sep 17, 2021 •
edited
Loading

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
@torgil
Make filetypes case insensitive on Windows
a8f6ad3 
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
Make Cpp filetypes case insensitive for windows
d1f92e4
@torgil torgil force-pushed the case-insensitive-windows-filetypes branch from a7f9d6c to d1f92e4 Compare October 6, 2021 07:34
meteorcloudy
meteorcloudy approved these changes Oct 6, 2021
View reviewed changes
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
torgil
torgil commented Oct 18, 2021
View reviewed changes
src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java
@@ -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
Make cpp assembly file extensions case sensitive again
f37ff44 
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 Gormo mentioned this pull request Oct 19, 2021
Closed
Gormo pushed a commit to Gormo/bazel that referenced this pull request Nov 9, 2021
Make cpp assembly file extensions case sensitive again
5e877f0 
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
Make cpp assembly file extensions case sensitive again
a6eea24 
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
@copybara-github
Make cpp assembly file extensions case sensitive again
edfe2a1 
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
@Bencodes
Make cpp assembly file extensions case sensitive again
eea81ac 
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
@foxandi foxandi mentioned this pull request Mar 18, 2022
Closed
fmeum pushed a commit to fmeum/bazel that referenced this pull request Jul 27, 2022
@fmeum
Make cpp assembly file extensions case sensitive again
11f2e24 
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 fmeum mentioned this pull request Jul 27, 2022
Merged
sgowroji pushed a commit to sgowroji/bazel that referenced this pull request Jul 27, 2022
@sgowroji
Make cpp assembly file extensions case sensitive again
2b5a43b 
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 sgowroji mentioned this pull request Jul 27, 2022
Closed
sgowroji pushed a commit that referenced this pull request Jul 27, 2022
@fmeum
Make cpp assembly file extensions case sensitive again (#15986)
a24d1bc 
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 <simon.bjorklen@gmail.com>
kkpattern added a commit to kkpattern/bazel that referenced this pull request Apr 17, 2023
@kkpattern
Make cpp file extensions case sensitive again
8e3fea8 
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 kkpattern mentioned this pull request Apr 17, 2023
Closed
copybara-service bot pushed a commit that referenced this pull request Apr 21, 2023
@kkpattern @copybara-github
Make cpp file extensions case sensitive again
e4990f8 
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
@kkpattern @fweikert
Make cpp file extensions case sensitive again
8bb3098 
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 FaBrand mentioned this pull request Jun 1, 2023
Merged
FaBrand pushed a commit to FaBrand/bazel that referenced this pull request Jun 1, 2023
@kkpattern @FaBrand
Make cpp file extensions case sensitive again
eb9a132 
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
@FaBrand @kkpattern
Make cpp file extensions case sensitive again (#18552)
e111cd6 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@limdor limdor limdor left review comments

@meteorcloudy meteorcloudy meteorcloudy approved these changes

Assignees

@meteorcloudy meteorcloudy

Labels
area-Windows Windows-specific issues and feature requests cla: yes team-Rules-CPP Issues for C++ rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@torgil @limdor @meteorcloudy @oquenchil