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

update ci to include windows #99

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open

update ci to include windows #99

wants to merge 49 commits into from

Conversation

stephprince
Copy link
Collaborator

@stephprince stephprince commented Sep 17, 2024

Fix #15.

These changes add support for windows to our GitHub Actions workflows. As part of these changes, I addressed some build errors and warnings:

  • linked bcrypt in the CMakeLists.txt on windows for boost-uuid functions. In think usually this should be auto-linked, but installing boost with vcpkg seems to disable this auto-linking.
  • split up long spec strings (specifically the nwb_file one) into multiple components to avoid strings being too long for the compiler. I'm not sure how conservative we want to be on the maximum string length
  • updated the SWMR tests
    • used a platform-independent sleep function
    • used conditional compiling for calling the reader executable
  • fixed various small warnings (unused variables, shadowed variables, implicit casting)

Note: squash when merging

@stephprince
Copy link
Collaborator Author

Everything is now be building/installing successfully and the tests are running.

However, the SWMR mode tests are failing on windows. I'm not sure what exactly might be causing these differences on the windows tests vs. the macOS and ubuntu tests, whether it is an issue with the SWMR mode setup or some other file permission issue. @oruebel do you have any ideas?

@stephprince stephprince marked this pull request as ready for review September 18, 2024 04:13
@oruebel
Copy link
Contributor

oruebel commented Sep 18, 2024

Not sure. Windows tends to lock file access more strongly so maybe the file isn't being closed correctly or opened in non-swmr read mode by the reader. There is a separate read mode for SWMR if I remember correctly.

@oruebel
Copy link
Contributor

oruebel commented Sep 20, 2024

@stephprince I synced the PR with the main branch. Just wanted to mention just in case, since there were a number of merge conflicts, but I believe I resolved them all ffffdc5 correctly

@oruebel
Copy link
Contributor

oruebel commented Sep 20, 2024

I believe I resolved them all ffffdc5 correctly

Spoken too soon. But I updated the tests and the build is working now. There is a failure on the tests, but I think that was already happening before the update.

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.

Add CI for Windows
2 participants