-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Bump veal submodule #6771
Bump veal submodule #6771
Conversation
|
Thanks for testing. I realize now that quite a bit has happened on our fork, this adds more than just MSVC support. We can merge it and cross our fingers or wait on some detailed testing. |
This may be superseded by #6758 |
feel free to merge this before #6758. That will make it easier to debug calf related issues in the future and keep that pr for enabling support alone, bumping can be done by this pr instead. |
If it were just a matter of MSVC changes, I'd merge this without question, but since there were several fast-forwards from upstream code changes, it increases the chance of a regression. I can merge this, but I will not have time to jump into tracking down regressions, so this is a risk. I just want another developer to OK this. |
Happened to take a look at the code again and saw that the majority changes are, in fact, msvc changes. Also, most of the other changes look harmless too. The only suspect possible of causing any regression is in the Edit : took another look and found that the remainder of the changes were either version bumps, xml changes or gui related (which we don't currently use as we have our own). Also about the Inviting @zonkmachine for testing. |
From a code perspective, no complaints against this (but testing would not harm). Note that in the recent days, I merged a few commits to calf's master, and there may follow some in the next week, as I am setting up a CMake base CI for it, including MSVC support. Maybe it is better to still wait? But I am OK with merging, too. |
The veal submodule has 3 new commits since this PR was opened, so they could be included too. Though if #7015 is merged first, that won't be needed. |
Adds MSVC support
See also LMMS/veal#9, LMMS/veal#5, LMMS/veal#6, (etc)