Re: [PATCH 1/2] xaudio2: Fix support for FAudio 19.06 and newer
Those names definitely look messy, but not quite messy enough… I’ll keep thinking of a good name, but for now (at least for demonstration purposes) I’ve gone ahead and used ‘BADABI’ to lay out how this is going to go down: https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135... <https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135fc4/include/FAudio.h#L662> https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135... <https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135fc4/include/FAPOFX.h#L159> So I’ve added the fixed functions and that’ll work for the rest of 2019 - this can safely be used for all of Wine 4.x and 5.0, and the function used by 4.3 through 4.7 (or 8 or 9, depending on when you want to move to the corrected function) can still build against the old function. It won’t work (and absolutely positively never will and never could have worked, just to emphasize one more time), but it’s at least there for compilation purposes. By the time 5.0 is out the proper function will be available and the 5.x series can use the new function, while 5.0.x maintenance releases can continue to use the BADABI function safely without having to worry about it getting lost before 6.0 is out. Once 6.0 is out, both the stable/unstable branches of Wine will be on the right function and the old functions won’t apply at that point. If someone really _really_ needs to do a multi-year bisect, they can pass --disable-faudio and save themselves the build time. If the XAudio2 subsystem is what’s being examined, they’re most likely looking at FAudio and not Wine. -Ethan
The idea of backwards compatibility is that a program should still be able to compile and run with any respective combination of old and new versions of the library. Renaming the old function breaks this. It also means that you want to load the new function at runtime rather than assuming it'll be present if compile-time support is there.
As for naming convention, a few suggestions are:
* FAudio_CommitChanges_a (e.g. libdwarf), or FAudio_CommitChangesA if you insist on title case * FAudio_CommitChangesBySet * FAudio_CommitChanges_v2 * FAudio_CommitChanges_ex (or FAudio_CommitChangesEx, but this has the risk of colliding with a future function Microsoft adds)
Personally I'm a fan of a descriptive name like CommitChangesBySet(), but ultimately it's your choice.
On Thu, 2 May 2019 at 21:59, Ethan Lee <elee(a)codeweavers.com> wrote:
If someone really _really_ needs to do a multi-year bisect, they can pass --disable-faudio and save themselves the build time. If the XAudio2 subsystem is what’s being examined, they’re most likely looking at FAudio and not Wine.
Nobody really wants to, but that kind of bisect is not uncommon, no.
On 5/2/19 10:29 AM, Ethan Lee wrote:
Those names definitely look messy, but not quite messy enough… I’ll keep thinking of a good name, but for now (at least for demonstration purposes) I’ve gone ahead and used ‘BADABI’ to lay out how this is going to go down:
https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135... <https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135fc4/include/FAudio.h#L662>
https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135... <https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135fc4/include/FAPOFX.h#L159>
So I’ve added the fixed functions and that’ll work for the rest of 2019 - this can safely be used for all of Wine 4.x and 5.0, and the function used by 4.3 through 4.7 (or 8 or 9, depending on when you want to move to the corrected function) can still build against the old function.
Apologies for butting in, but I'm not sure this is any better. Code that's written to call FAudio_CommitChanges would expect this function to have the same specified behavior after an update. If code needs updating to use the new version, having a separate FAudio_CommitChangesBADAPI function seems a bit pointless; for the same effort of changing the call to restore old behavior, you could just ensure FAudio_CommitChanges is used correctly (unless the behavior change is that significant, in which case it really should be a new function). When it comes to ABI changes, it should represent a massive change for the interface and be a complete refresh. If you have libfaudio.so.0 and libfaudio.so.1, for example, what happens if a process tries to pull in both (e.g. the main app links against libfaudio.so.1, but the app also links against libfoo.so which links to libfaudio.so.0)? Now you have two separate FAudio_CommitChanges symbols with two different behaviors, and something will break. I've actually run into this problem with SDL, where I link against SDL2 and SDL_sound, but on some distros SDL_sound links against SDL1; it all compiles and links fine, but misbehaves at runtime for no immediately obvious reason (if I didn't know SDL_sound might be linked against SDL1, while I link against SDL2, I'd be baffled as to why seemingly correct calls are just failing for some people and not others).
Ethan Lee <elee(a)codeweavers.com> writes:
Those names definitely look messy, but not quite messy enough… I’ll keep thinking of a good name, but for now (at least for demonstration purposes) I’ve gone ahead and used ‘BADABI’ to lay out how this is going to go down:
https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135...
https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135...
So I’ve added the fixed functions and that’ll work for the rest of 2019 - this can safely be used for all of Wine 4.x and 5.0, and the function used by 4.3 through 4.7 (or 8 or 9, depending on when you want to move to the corrected function) can still build against the old function. It won’t work (and absolutely positively never will and never could have worked, just to emphasize one more time), but it’s at least there for compilation purposes. By the time 5.0 is out the proper function will be available and the 5.x series can use the new function, while 5.0.x maintenance releases can continue to use the BADABI function safely without having to worry about it getting lost before 6.0 is out. Once 6.0 is out, both the stable/unstable branches of Wine will be on the right function and the old functions won’t apply at that point.
That's not an improvement. You should not be planning to break the ABI ever, and certainly not on a yearly basis. It's unfortunate that you can't name it FAudio_CommitChanges because that name is taken by the old one, but you should get over it, pick a proper name, and plan on maintaining it forever. There's no reason to rename it again next year, that's just gratuitous breakage. -- Alexandre Julliard julliard(a)winehq.org
That’s not an improvement.
Does this mean I can go back to my original plan then? This really sucks, I get it, but if we can’t come up with some kind of compromise to make this as un-sucky as possible then I’ll go with what my other users will understand the best by default. They can handle breakages WAY better by the sound of it, so if it’s really that painful then I need to start seeing some suggestions for a real plan other than “leave the catastrophic bug in” which benefits literally nobody but Wine’s build (not even Wine as a whole, just the build). -Ethan
On May 2, 2019, at 5:17 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Ethan Lee <elee(a)codeweavers.com <mailto:elee(a)codeweavers.com>> writes:
Those names definitely look messy, but not quite messy enough… I’ll keep thinking of a good name, but for now (at least for demonstration purposes) I’ve gone ahead and used ‘BADABI’ to lay out how this is going to go down:
https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135...
https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135...
So I’ve added the fixed functions and that’ll work for the rest of 2019 - this can safely be used for all of Wine 4.x and 5.0, and the function used by 4.3 through 4.7 (or 8 or 9, depending on when you want to move to the corrected function) can still build against the old function. It won’t work (and absolutely positively never will and never could have worked, just to emphasize one more time), but it’s at least there for compilation purposes. By the time 5.0 is out the proper function will be available and the 5.x series can use the new function, while 5.0.x maintenance releases can continue to use the BADABI function safely without having to worry about it getting lost before 6.0 is out. Once 6.0 is out, both the stable/unstable branches of Wine will be on the right function and the old functions won’t apply at that point.
That's not an improvement. You should not be planning to break the ABI ever, and certainly not on a yearly basis.
It's unfortunate that you can't name it FAudio_CommitChanges because that name is taken by the old one, but you should get over it, pick a proper name, and plan on maintaining it forever. There's no reason to rename it again next year, that's just gratuitous breakage.
-- Alexandre Julliard julliard(a)winehq.org <mailto:julliard(a)winehq.org>
On 5/2/19 7:46 PM, Ethan Lee wrote:
That’s not an improvement.
Does this mean I can go back to my original plan then? This really sucks, I get it, but if we can’t come up with some kind of compromise to make this as un-sucky as possible then I’ll go with what my other users will understand the best by default. They can handle breakages WAY better by the sound of it, so if it’s really that painful then I need to start seeing some suggestions for a real plan other than “leave the catastrophic bug in” which benefits literally nobody but Wine’s build (not even Wine as a whole, just the build).
What exactly is the "catastrophic bug" here? That CommitChanges() won't do anything? That was already the case; why is it suddenly worse now that a working implementation exists? Or that it has the wrong signature? Why is that catastrophic then, if it doesn't do anything anyway?
On Thu, May 2, 2019 at 12:29 PM Ethan Lee <elee(a)codeweavers.com> wrote:
Those names definitely look messy, but not quite messy enough… I’ll keep thinking of a good name, but for now (at least for demonstration purposes) I’ve gone ahead and used ‘BADABI’ to lay out how this is going to go down:
https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135...
https://github.com/FNA-XNA/FAudio/blob/16d1dfeb92a4ae18eacb67c3dcf3f75de5135...
So I’ve added the fixed functions and that’ll work for the rest of 2019 - this can safely be used for all of Wine 4.x and 5.0, and the function used by 4.3 through 4.7 (or 8 or 9, depending on when you want to move to the corrected function) can still build against the old function. It won’t work (and absolutely positively never will and never could have worked, just to emphasize one more time), but it’s at least there for compilation purposes. By the time 5.0 is out the proper function will be available and the 5.x series can use the new function, while 5.0.x maintenance releases can continue to use the BADABI function safely without having to worry about it getting lost before 6.0 is out. Once 6.0 is out, both the stable/unstable branches of Wine will be on the right function and the old functions won’t apply at that point.
If someone really _really_ needs to do a multi-year bisect, they can pass --disable-faudio and save themselves the build time. If the XAudio2 subsystem is what’s being examined, they’re most likely looking at FAudio and not Wine.
-Ethan
Labeling the newer function as BADAPI strikes me as even worse. I'd expect something labeled BADAPI to be proscribed rather than prescribed. I think this approach is a matter of looking at compatibility the wrong way. I think it doesn't make sense to treat the erroneous function as so wrong that it needs to be completely removed from the public library, nor I do see why it's absolutely necessary that the correct implementation of CommitChanges actually have the exact name CommitChanges(). API compatibility isn't something you keep as a temporary measure until you think that everyone has updated your library; it's something you keep for as long as possible, until the combined weight of design mistakes and errors makes it more worthwhile just to scrap the whole interface and recreate it. Incrementing the major version shouldn't be something you do every year just because time passes, but rather something you do only when you have to. (That's not to say you shouldn't call it 20.x and 21.x etc. after the year, but the library major version should remain the same.) An incorrect parameter signature doesn't make that necessary, inasmuch as FAudio isn't an ABI-compatible drop-in replacement for XAudio but rather a tool to recreate its logic and to allow easy porting. Anyone who's interested in porting can rely on documentation to tell them that CommitChanges() is deprecated in favor of CommitChangesBySet(), if the missing parameter isn't already enough of a clue.
participants (6)
-
Alexandre Julliard -
Chris Robinson -
Ethan Lee -
Henri Verbeet -
Zebediah Figura -
Ztirfe Elgnid