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 cpp assembly file extensions case sensitive again #14131

Closed

Conversation

Gormo
Copy link

@Gormo Gormo commented Oct 19, 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.

@Gormo Gormo requested a review from lberki as a code owner October 19, 2021 07:59
@google-cla google-cla bot added the cla: yes label Oct 19, 2021
@google-cla google-cla bot added cla: no and removed cla: yes labels Oct 19, 2021
@Gormo Gormo force-pushed the fix_s_S_case_insensitive_on_Windows branch from bdb45e3 to 6fe58ee Compare October 19, 2021 09:30
@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 19, 2021
Copy link
Contributor

@oquenchil oquenchil left a comment

Choose a reason for hiding this comment

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

LGTM

@Gormo
Copy link
Author

Gormo commented Nov 8, 2021

@lberki, can we merge this so we don't need to maintain this patch locally when doing uplifts?

@oquenchil
Copy link
Contributor

@buildbreaker2021 will take over the review and import.

@Gormo Gormo force-pushed the fix_s_S_case_insensitive_on_Windows branch from 6fe58ee to 5e877f0 Compare November 9, 2021 14:57
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 force-pushed the fix_s_S_case_insensitive_on_Windows branch from 5e877f0 to a6eea24 Compare November 9, 2021 15:09
Copy link
Contributor

@buildbreaker2021 buildbreaker2021 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

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

This PR seems to negate the assumptions made in WindowsOsPathPolicy.endsWith, that Windows don't know about character case (upper/lower). https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/vfs/WindowsOsPathPolicy.java;l=213;drc=9cb59369a84d9328c2929eefcb58c1b972ab15f2

Having a special case for Windows in CppFileTypes introduces a tech debt in my opinion - there's lack of comments (in this PR) why there is a need for a special case here. Please add a comment with a link to an issue.

I think the right thing to do is to remove the assumption (of case insensitivity) in WindowsOsPathPolicy and then fix the locations where poorly behaved toolchains create either lower or upper cased suffices.

@torgil
Copy link
Contributor

torgil commented Nov 15, 2021

I think the right thing to do is to remove the assumption (of case insensitivity) in WindowsOsPathPolicy and then fix the locations where poorly behaved toolchains create either lower or upper cased suffices.

This would reintroduce the issues in #14005. The "right thing" question in there was never properly discussed but you could reason that a new windows user would have a hard time to "fix" third-party libraries for casing (Microsoft clibs package in my case) or make Windows-specific case-by-case additions to CppFileTypes.

@comius
Copy link
Contributor

comius commented Nov 15, 2021

I think the right thing to do is to remove the assumption (of case insensitivity) in WindowsOsPathPolicy and then fix the locations where poorly behaved toolchains create either lower or upper cased suffices.

This would reintroduce the issues in #14005. The "right thing" question in there was never properly discussed but you could reason that a new windows user would have a hard time to "fix" third-party libraries for casing (Microsoft clibs package in my case) or make Windows-specific case-by-case additions to CppFileTypes.

Sure, I agree.

Would you mind just adding a comment to the code, that "FileType is extended to use case-sensitive comparison also on Windows". I don't want the next developer hitting their head on it.

When this piece of code is rewritten to Starlark, we won't have the luxury of extending FileType, all file types will need to be listed case-sensitively. Perhaps special cased for linux/windows/macos one level higher.

I'm not sure if it's a good or a bad news we only need to special case this one special operating system :)

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 15, 2021
@Gormo Gormo requested a review from comius November 15, 2021 19:56
@Gormo Gormo force-pushed the fix_s_S_case_insensitive_on_Windows branch from 110be21 to 7094ec4 Compare November 19, 2021 09:22
@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 19, 2021
@Gormo
Copy link
Author

Gormo commented Nov 23, 2021

Ok for merge now?

@bazel-io bazel-io closed this in edfe2a1 Nov 24, 2021
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
Copy link
Collaborator

fmeum commented Jul 27, 2022

@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 Jul 27, 2022
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 sgowroji added the team-Rules-CPP Issues for C++ rules label Jul 27, 2022
@sgowroji
Copy link
Member

@bazel-io fork 5.3.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 Jul 27, 2022
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants