-
Notifications
You must be signed in to change notification settings - Fork 758
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
Remove potentially unused qt5 dependencies #3472
Remove potentially unused qt5 dependencies #3472
Conversation
One request: can we either leave in the Qt5OpenGL dependency, or leave the code but comment it out? Sensors and Positioning are neither here nor there, but there's a decently likelihood that we might start using OpenGL in the future. Cmake is already a hairy beast that very few of the core devs are comfortable modifying, so leaving a breadcrumb trail for this would be helpful. |
I agree, at the very least leave it commented. Also quick note that this might mean we can remove some dependencies from CI scripts and Readmes |
Can I conclude based on the passed CI runs that these dependencies are indeed unused?
I can comment that one out if that's preferred? I'd say it's in the git history so that wouldn't be needed but it's your codebase :)
I've found this https://github.com/supercollider/supercollider/blob/develop/.travis/before-install-linux.sh#L9 |
Nope, just there and then the corresponding place in README_LINUX. The other two OSs use monolithic installs of Qt. Thanks! |
I can update the Linux build guides that are in the Wiki. I think the Rpi and BBB build guides on the website might need to be changed as well. |
ahh, good point |
@brianlheim @scztt you prefer I comment out the Qt5OpenGL changes instead of removing? |
yes, thanks for confirming. I get what you're saying about git history but unfortunately on a project this large it's easy for things like that to get lost |
@brianlheim OK, thanks for the confirmation. I'll update the PR this weekend. |
@simonvanderveldt - can you take another look at this when you get a chance please? |
@brianlheim Sure, sorry for the radio silence. I'll put in in my agenda for this weekend, is that OK? |
No problem! Just wanted to make sure this doesn't go stale. |
17fa4a1
to
880361c
Compare
All right, updated the Qt5OpenGL commit to just comment out (as best as I could) the changes. Though t.b.h. I'm not sure how much these comments make sense. Also removed the |
Thanks!
I understand where you're coming from, but from personal experience not everyone will be able to understand and modify these files as easily as you did. Not really a big deal either way, just trying to give some explanation. |
880361c
to
26c7f5a
Compare
Just noticed I forgot to also update |
PR to see if these Qt5 dependencies can be removed as discussed in #3471