Skip to content

Conversation

@emmenlau
Copy link
Member

Currently the cmake-generated header config.h is not used on MSVC. However it can be helpful to have the option to use this header, for example to detect the presence of newer Windows SDK features (like used in #2327).

I've tested this addition for over a year successfully on various combinations of Windows and did not find any issues or problems. I've also tried to make sure that previous Windows defines remain available unless overridden by cmake (which should be a good thing).

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@emmenlau emmenlau force-pushed the bda_use_cmake_config_on_msvc branch from e9855f2 to 57554d2 Compare August 29, 2021 17:06
@emmenlau emmenlau assigned emmenlau and unassigned emmenlau Aug 29, 2021
@emmenlau
Copy link
Member Author

The failed test on AppVeyor MSVC 2015 seems unrelated.

Copy link
Member

@fishy fishy left a comment

Choose a reason for hiding this comment

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

I'm not familiar with MSVC nor cmake but this certainly passes a lot more travis tasks than before so looks good to me :)

@emmenlau emmenlau force-pushed the bda_use_cmake_config_on_msvc branch from 57554d2 to 5a64c71 Compare August 30, 2021 09:42
@emmenlau
Copy link
Member Author

Rebased on latest master to fix .NET installation issues on Travis

@emmenlau
Copy link
Member Author

Remaining errors on Travis are all unrelated :-)

@emmenlau emmenlau merged commit 5b25b99 into apache:master Aug 31, 2021
@emmenlau emmenlau deleted the bda_use_cmake_config_on_msvc branch August 31, 2021 12:52
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