ctubbsii commented on PR #2649: URL: https://github.com/apache/thrift/pull/2649#issuecomment-2289839464
> > Sorry that this is such a troublesome issue for such a simple problem. But I am under the impression that there are two contradictory choices: > > ``` > > * use lowercase spelling, which helps for cross-compilation, but breaks case-sensitive Windows installations, and > > > > * use the current spelling, which breaks cross-compilation, but helps for case-sensitive Windows installations. > > ``` > > Agreed that these are contradictory, but pragmatically I would not give them equal footing. There are several people cross-compiling, and the current codebase has some setup for cross-compiling already. The case-sensitive Windows installations seem hypothetical to me, and even if it breaks in some corner case, the question is how often such a setup is used. If I search the apache org for the include: https://github.com/search?q=org%3Aapache+%23include+%3Cwindows.h%3E&type=code, I roughly see 80-90% of the hits use the lowercase variant, so there seem no issues on those repositories. > > So I would just go forward with using lowercase everywhere, but then again, I'm biased here ;) I agree that the case-sensitive Windows case is very hypothetical. I'd fix it for the 90% use case using lower case exclusively, and leave it to the outliers to patch their build if they are doing something with a case-sensitive filesystem on Windows and have differently cased files. @emmenlau 's suggestion to have build checks for the correct defines is fine, if somebody wanted to pursue it for those outlier situations, but I don't think it should block this simple fix from being merged. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@thrift.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
