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

Ensure that public headers are prefixed with 'InfluxDB". #263

Closed
wants to merge 1 commit into from

Conversation

BillyONeal
Copy link

In vcpkg's registry, before microsoft/vcpkg-tool#1483 , we missed that influxdb-cxx conflicts with libproxy, because the former spells the header include/Proxy.h while the latter spells it include/proxy.h. This caused the ports to silently damage each other on case insensitive file systems, such as on Windows and macOS.

Given that the names "Proxy.h", "Transport.h", and "Point.h" are all extremely likely to conflict with other users, and influxdb-cxx already seems to have a convention for header names to be prefixed with "InfluxDB", we, the vcpkg registry maintainers, believe it would be best if these headers were renamed to resolve the conflict.

In vcpkg's registry, before microsoft/vcpkg-tool#1483 , we missed that `influxdb-cxx` conflicts with `libproxy`, because the former spells the header `include/Proxy.h` while the latter spells it `include/proxy.h`. This caused the ports to silently damage each other on case insensitive file systems, such as on Windows and macOS.

Given that the names "Proxy.h", "Transport.h", and "Point.h" are all extremely likely to conflict with other users, and `influxdb-cxx` already seems to have  a convention for header names to be prefixed with "InfluxDB", we, the vcpkg registry maintainers, believe it would be best if these headers were renamed to resolve the conflict.
@offa
Copy link
Owner

offa commented Oct 10, 2024

🙈 Oh dear …

Thank you for reporting.

The convention is more or less class name = file name.

I would suggest moving the files to a subdirectory called "include/InfluxDB/" instead of renaming them. The CMake files are installed in a similar subdirectory already.

@BillyONeal
Copy link
Author

I would suggest moving the files to a subdirectory called "include/InfluxDB/" instead of renaming them. The CMake files are installed in a similar subdirectory already.

That would also work 👍. I'm less confident in making that change correctly myself :/

@offa
Copy link
Owner

offa commented Oct 10, 2024

No problem, I'll take care of it.

@offa
Copy link
Owner

offa commented Oct 10, 2024

Could you give #264 a try?

@BillyONeal
Copy link
Author

Obsoleted by #264

@BillyONeal BillyONeal closed this Oct 21, 2024
@BillyONeal BillyONeal deleted the rename-headers branch October 21, 2024 17:53
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.

2 participants