Skip to content

Conversation

tringenbach
Copy link
Contributor

This change addresses item #1596

This changes the path normalization to not lowercase paths on Mac (darwin).

The purpose of this change

The C++ extension treats paths as case sensitive on MacOS. (In reality, sometimes they are, but it depends, usually not).

When CMakeTools converts them to lowercase and sends the include paths over, they don't work, and all the #include's that reference files in those paths get red squiggles.

I do not understand why converting to lower case would be desirable, even if it didn't break. The OS is at least case preserving, so the"normalized or canonical form really is the form the mixed case.

@gcampbell-msft
Copy link
Collaborator

@sean-mcmanus @Colengms Any thoughts on this PR?

It sounds like it's possible that the C++ extension should be treating mac paths as case-insensitive on the C++ side

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Mar 7, 2025

@gcampbell-msft We treat mac paths as case sensitive. We only normalize (change the case) of paths on Windows (we change it to uppercase internally). The PR changes seems good to me. I don't think lowercasing paths on mac is a good idea.

@tringenbach
Copy link
Contributor Author

FWIW, Disk Utility on MacOS does allow me to create a case sensitive APFS filesystem.

image

Perhaps more practical, when I saw lowercase paths in the logs in VSCode, that jumped out at me as "wrong". So even if you changed the C++ extension to accept it, it would "look wrong" and get blamed incorrectly for other problems. At least that's my logic.

I'm happy with whatever works though :)

@gcampbell-msft
Copy link
Collaborator

I'm happy to take this then @tringenbach could you make a changelog update and pull it up to date with main? Then, I'll approve

@tringenbach tringenbach force-pushed the feature/fix-mac-include-paths-lowercase branch from 4b14d09 to 4edd184 Compare March 8, 2025 00:30
@tringenbach tringenbach force-pushed the feature/fix-mac-include-paths-lowercase branch from 4edd184 to 7090763 Compare March 8, 2025 00:32
@tringenbach
Copy link
Contributor Author

@microsoft-github-policy-service agree

@tringenbach
Copy link
Contributor Author

I'm happy to take this then @tringenbach could you make a changelog update and pull it up to date with main? Then, I'll approve

Sure! How's that?

@gcampbell-msft
Copy link
Collaborator

@tringenbach Slightly updated, and approved, I will merge once tests and builds pass. Thanks!

gcampbell-msft
gcampbell-msft previously approved these changes Mar 10, 2025
@gcampbell-msft
Copy link
Collaborator

Already passed tests and build, just a merge conflict resolution for the changelog, merging.

@gcampbell-msft gcampbell-msft merged commit 4dc5400 into microsoft:main Mar 10, 2025
1 check passed
hippo91 pushed a commit to hippo91/vscode-cmake-tools that referenced this pull request Mar 29, 2025
* Fix lowercase include paths on Mac

* Update CHANGELOG for version 1.21

---------

Co-authored-by: Garrett Campbell <[email protected]>
fajkomix1990 pushed a commit to fajkomix1990/mati---glowny that referenced this pull request Jul 2, 2025
* Fix lowercase include paths on Mac

* Update CHANGELOG for version 1.21

---------

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

Successfully merging this pull request may close these issues.

3 participants