-
Notifications
You must be signed in to change notification settings - Fork 135
Multi-platform & multi-version CI #176
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
Conversation
vaind
commented
Jan 25, 2021
•
edited
Loading
edited
- closes CI on macOS and Windows #80
- closes CI - test all supported dart/flutter versions #82
e9305c3
to
16e5905
Compare
include relative and absolute paths where install.sh puts the lib
16e5905
to
fb6fb10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some questions.
lib/src/bindings/flatbuffers.dart
Outdated
DynamicLibrary lib; | ||
if (Platform.isWindows) { | ||
// DynamicLibrary.process() is not available on Windows | ||
lib = DynamicLibrary.open('msvcr100.dll'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this workaround coming from? Appears to be a C library. Does this require installation of Microsoft Visual C++ 2015 Redistributable (x64) like mentioned at https://docs.objectbox.io/java-desktop-apps#native-libraries? Should this be noted in a README?
The GitHub Windows instance has a bunch of versions installed, so I guess most are fine. https://github.com/actions/virtual-environments/blob/win19/20201210.0/images/win/Windows2019-Readme.md#microsoft-visual-c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this workaround coming from?
Nowhere in particular, I've just searched what libary defines memset
on windows. Turns out it's mscvrt.dll
or mscvr100.dll
. Yes it may be fragile even though the library is pretty common, some users might not have it... I'll add another fallback layer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add this in a comment then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, also added a comment