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

Cleanup: Remove CDUtils #11474

Merged
merged 1 commit into from Jan 29, 2023
Merged

Conversation

MayImilae
Copy link
Contributor

@MayImilae MayImilae commented Jan 23, 2023

This is a cleaning followup for #11456, Remove Boot from DVD Backup. CDUtils is a self-described shameless rip of libcdio, a linux library for accessing drives. It did a lot of heavy lifting for the Boot from DVD Backup feature, despite not being directly connected to it and not giving any errors when I removed Boot from DVD Backup. That and the fact that it only mentions CDs, not DVDs, is why I missed this in the initial PR. Now it probably does absolutely nothing, and even if it does do something it would be broken without DriveBlob.

This PR is specifically a removal of CDUtils. There are potentially more Boot from DVD Backup bits remaining, but DiscIO and DVDInterface are a little confusing to me and I didn't want to rip out anything that Dolphin needs to boot ISOs. So there is possibly more cleanup left to do, but that will be for another PR.

Please review!


Also... if anyone decides to reimplement Boot from DVD Backup, please rebuild it into something better rather than restoring this mess. The way cdutils+driveblob worked and interacted together (or more specifically, the complete lack thereof) was very awkward and weird. It would be so much better if all physical drive shenanigans were together in one file, and it would be much cleaner too.

@MayImilae
Copy link
Contributor Author

@JosJuice I especially would like for you to review this.

@MayImilae MayImilae force-pushed the cleanup-remove-cdutils branch 2 times, most recently from ffeb5fd to 9a88804 Compare January 23, 2023 11:53
@MayImilae
Copy link
Contributor Author

Tested on Kubuntu, macOS 12, and Windows 11. Games still boot!

@JosJuice
Copy link
Member

JosJuice commented Jan 24, 2023

Putting the question of whether the disc drive feature should be removed aside: The code change LGTM, but I have some feedback regarding the commit message.

This is a cleaning followup for #11456, Remove Boot from DVD Backup. CDUtils is a self-described shameless ripoff of cdio, a linux library for accessing drives. It did a lot of the heavy lifting for the Boot from DVD Backup feature, despite not being connected to it in any way and not giving any errors when removing Boot from DVD Backup. That and the fact that DVD is not anywhere within it is why I missed this in the initial PR.

The writing here makes sense, but there are no line breaks in the text. For better or worse, in commit messages you're supposed to place line breaks manually.

CDUtil has fingers in lots of things. Despite the many files changed here, this is a fairly conservative removal. At this level it gets really hard to distinguish things necessary for actual physical drives and things needed for emulated drives, and I didn’t want to rip out anything that Dolphin needs to boot ISOs. There is likely still some DVD bits I missed though, though this is the last one I’m aware of.

The code change you made seems like a perfectly standard removal of something that isn't being used anymore. You removed the CDUtils files, all includes of them, and all uses of functions defined in them. You didn't need to make changes in all that many places either. I don't think it makes sense to say that CDUtils had fingers in lots of things or that this is a conservative removal.

This absolutely needs thorough review.

Personally I put things like this in the PR description but not the commit message. After all, by the time the PR is merged, the commit will have been properly reviewed and thus there's no reason to have this text in the commit anymore. But maybe that's just me.

@MayImilae
Copy link
Contributor Author

MayImilae commented Jan 24, 2023

Ahh I was not aware there was any sort of commit message etiquette. In fact i wasn't really aware there was a difference between commit message and PR description! I'll take care of it.

EDIT: Done!

@JosJuice
Copy link
Member

Would prefer if the commit message is wrapped at 72 characters instead of at the end of each sentence, but if someone else wants to merge this, I'm not going to block it.

@MayImilae MayImilae force-pushed the cleanup-remove-cdutils branch 2 times, most recently from 5a340f7 to 4b74ffc Compare January 28, 2023 07:51
This is a cleaning followup for dolphin-emu#11456.
@MayImilae
Copy link
Contributor Author

The commit message is now under 72 characters per line. Everyone writing to Dolphin on their IBM 3270 will be pleased.

Also rebased so there are no longer any spurious fifoci errors.

@delroth delroth merged commit 2eda76c into dolphin-emu:master Jan 29, 2023
14 checks passed
@MayImilae MayImilae deleted the cleanup-remove-cdutils branch January 29, 2023 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants