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

Fix/fix the cpp build on windows #109

Merged
merged 9 commits into from
Jun 18, 2024

Conversation

chrisdutz
Copy link
Contributor

This PR adds the usage of the cmake-maven-plugin back as well as addresses some code-level changes needed in order to compile TsFile CPP on Windows systems.

Manually tested with mvnw -P with-cpp clean install on:

  • Windows aarch64
  • Windows intel
  • Linux aarch64
  • Linux intel
  • MacOS aarch64
  • MacOS intel

@HTHou
Copy link
Contributor

HTHou commented Jun 13, 2024

Thanks Chris! Consider enabling the workflow on windows then?

if [[ "${{ matrix.os }}" == "windows-latest" ]]; then
./mvnw${{ steps.platform_suffix.outputs.platform_suffix }} -P with-java clean verify
else
./mvnw${{ steps.platform_suffix.outputs.platform_suffix }} -P with-java,with-cpp clean verify
fi

@ColinLeeo
Copy link
Contributor

Excellent job!

@chrisdutz
Copy link
Contributor Author

I guess the probably most important part is to either ask the author of the code snippet to flag his repo as Apache licensed, or to send us a copy of it with an apache license header ... right now it's technically under the GitHub default license and that's not quite compatible with the Apache one.

@chrisdutz
Copy link
Contributor Author

Ok ... Windows GitHub Action builds enabled and working (Had to do a minor addition to one of the functions in order to make it work however)

@chrisdutz
Copy link
Contributor Author

Ok... I manged to reach the author of the patch and he just set the license of his repo to apache 2.0, so I guess we're good to merge the pr

cpp/src/file/read_file.cc Outdated Show resolved Hide resolved
@chrisdutz
Copy link
Contributor Author

So? We good to merge?

@HTHou
Copy link
Contributor

HTHou commented Jun 17, 2024

So? We good to merge?

@jt2594838

@jt2594838 jt2594838 merged commit c6af3c3 into apache:develop Jun 18, 2024
13 checks passed
JackieTien97 pushed a commit that referenced this pull request Jun 21, 2024
* FIX: Get the windows build working again

* FIX: Updated the cmake-generator to match that of IoTDB.

* FIX: Reverted to MinGW as there were errors with VisualStudio 17

* fix: Adjusted the code to compile on linux, mac and windows.

* feat: Enabled the cpp build on windows in github actions

* fix: Tried to fix a compiler error (which however doesn't make much sense)

* Removed the comments of license uncertainty.

* feat: Removed the unneeded download
JackieTien97 pushed a commit that referenced this pull request Jun 24, 2024
* FIX: Get the windows build working again

* FIX: Updated the cmake-generator to match that of IoTDB.

* FIX: Reverted to MinGW as there were errors with VisualStudio 17

* fix: Adjusted the code to compile on linux, mac and windows.

* feat: Enabled the cpp build on windows in github actions

* fix: Tried to fix a compiler error (which however doesn't make much sense)

* Removed the comments of license uncertainty.

* feat: Removed the unneeded download
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.

4 participants