-
Notifications
You must be signed in to change notification settings - Fork 14
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
lsl version in appveyor? #33
Comments
Yeah that is odd, pretty old too. I don't have an active Matlab license at the moment so I can't help. I have no objection to you completely changing the CI system. |
@cboulay, I am sure you don't ;-). I need a little more context, though. I am not very familiar with appveyor etc. so I am not really sure what I am looking at. As far as I can tell, the script simply tries to run build_mex and that is it. |
I'm pretty sure AppVeyor uses PowerShell by default, so any commands you see there should work in powershell. I think I just saw the MOxUnit github action and I think that looks pretty promising. (I think you meant to link this: https://github.com/joergbrech/moxunit-action) |
Yes, that is the link I meant. Had the wrong copy in my clipboard. OK, so it looks like I can just bash out the travis stuff and setup a github actions using MOxUnit. It looks like I can just run it on |
It probably doesn't buy you or I anything, but it's essential when we have a community contributor and we can't verify that they fully tested their changes in their PR. That being said, our Matlab test suite is pretty barren at the moment so we can't really claim this as a benefit. As for whether or not we build the mex files in CI runners and provide them as archives, that's complicated too. I'd really like to because it means the user doesn't have to install a compiler, making sure that the compiler they install matches the version of matlab they are targeting. However, it's very difficult to maintain. We'd probably be better off taking the time we might spend writing CI scripts and spend it on documentation. That's more likely to be compatible with the various configurations. All that to say I'm fine with disabling CI for this repo. |
I think we should disable what is currently there as it doesn't seem to work anyway. You are right that it does verify that a PR at least doesn't fail to build. I guess I am just looking at the practical fact that the Matlab very wrapper rarely changes and that very few 3rd part devs are interested in using this wrapper. In my experience, most of the Matlab users are not coders anyway. If I have time, I will play around with the MOxUnit thing. If it is as easy as advertised, then I will push it in. Maintaining compiled packages for this wrapper is not something I am interested in doing---even if I had the time to spend doing it. Ideally, Mathworks would want to support this, but I don't think that liblsl-Matlab is big enough for them to warrant the resources. I think the documentation for building it is already pretty good. |
Unless I missed a major change in MathWork's business model, running Matlab in public CI systems isn't feasible and in my experience Octave is close enough to catch most compatibility issues. The Appveyor CI config is from 2018 and not that functional, so it could be removed. |
I just did a PR to update the lsl version in lsl_get_dll (#32). The CI checks failed. No idea why. It built just fine on my Windows 10 machine with Matlab 2020b. The CI is configured for octave. I am not sure what that is actually doing but I don't believe that running tests on liblsl-Matlab against octave is correct. There is something called MOxUnit that may be more appropriate: https://github.com/scottclowe/matlab-continuous-integration.
Also, the lsl version in the .travis.yml code is very odd: https://github.com/labstreaminglayer/liblsl-Matlab/blob/master/.travis.yml#L5. Is there any reason not to test against the latest version?
The text was updated successfully, but these errors were encountered: