0003401: DOOMSEEKER_ABI mismatching plugins still pop up unknown procedure errors.

Notes
(0020461) Pol M (developer) 2019-03-31 15:46 edited on: 2019-03-31 15:59	Looks like right now it only shows two similar error messages. I'm gonna disable one of them, and leave the other. Following the "steps to reproduce", I have not managed to get said results. pr

(0020470) Zalewa (developer) 2019-04-03 11:07	The pr is not enough as the problem is directly related to Windows loading the DLL and auto-generating the pop-ups. The displayed errors are not generated by us. I don't even know if this is solvable without parsing the DLL's import table before passing them to LoadLibrary() but maybe WinAPI provides some method of capturing those rough Windows errors and handling them gracefully.

(0020471) Pol M (developer) 2019-04-03 13:14	Oh! That explains why on wine it was working. I'll investigate this issue on a native windows installation.

(0020496) Pol M (developer) 2019-04-16 18:38	Okay, I've found an easy way to handle this: SetErrorMode Unfortunately it won't allow us to know what errors are we suppressing. That said, since we can disable exclusively errors related to the engines, I'm not that worried about suppressing important info. PR

(0020514) Zalewa (developer) 2019-04-20 06:55 edited on: 2019-04-20 06:56	Oh the woes of see plus plus. The commit that disables SEM_FAILCRITICALERRORS does get rid of the invasive popup dialog boxes indeed. However, the #define DOOMSEEKER_ABI_VERSION still remains useless on Windows. Let's see: a DLL with an incompatible ABI refuses to load, right? If it refuses to load, then we cannot compare the ABI in the DLL to the #define in Doomseeker. Therefore, the #define has no chance to do anything. However, if you think about it, does it really need to? Let's consider: 1. Since Windows already detects ABI incompatibility for us we already get the "feature" of an incompatible DLL not being loaded. 2. Since DLLs are not loaded we're already safe from incompatibilities causing crashes. 3. The obnoxious error popups are we gone. There's now a somewhat cryptic "error 127" in the logs, which is not so bad for programmers, but most users won't know what it means. We could improve that a bit by checking if GetLastError() returns ERROR_PROC_NOT_FOUND and print a better log message that directly states that the plugin is not in the correct version. 4. With the unusable #define what we can't do is to display Doomseeker's and plugin's ABI version numbers so that the user knows which one is newer. But, how useful would that be, really?

(0020515) Zalewa (developer) 2019-04-20 07:26	I merged the PR as the commit here as it is a step in the right direction.

(0020517) Pol M (developer) 2019-04-20 11:53	Quote from Zalewa We could improve that a bit by checking if GetLastError() returns ERROR_PROC_NOT_FOUND and print a better log message that directly states that the plugin is not in the correct version. That's an excellent idea. I'd manually add an exception to code 127 and add something like "perhaps this is an old engine? Check for newer updates!" (Something that kinda states what the problem could be and how to solve it) Quote from Zalewa what we can't do is to display Doomseeker's and plugin's ABI version numbers so that the user knows which one is newer. But, how useful would that be, really? While it's true that it would be interesting to show the info, the end user will solve the problem by updating anyway, so I'd say that it is not that bad that they lose that info. I find it highly unlikely that someone would downgrade to an older version of doomseeker and use newer engines, so the issue will almost always appear when people update doomseeker partially.

(0020520) Zalewa (developer) 2019-04-21 13:27	The actual problems with ABI incompatibility will arise if someone decides to release a plugin that will be hosted outside of Doomseeker's source control and Doomseeker's release cycle. In such case it may be useful for the user to know if plugin is for an older or a newer version of Doomseeker. If such plugin appears we will also need to be much more rigorous about Doomseeker's ABI instead of breaking it freely whenever the current Plugin API gets in the way of the development.

(0020524) Pol M (developer) 2019-04-21 16:05	Quote from Zalewa In such case it may be useful for the user to know if plugin is for an older or a newer version of Doomseeker. Yeah, that will be a small problem. That said, I'd imagine that the developer will list the compatible doomseeker versions, and the slow release cycle will help us here, giving said developer time to adjust to the new ABI or mark the new Doomseeker version as compatible. It's a shame that the info will not be displayed, because it's very likely that they will use Windows to develop plugins. To mitigate a little the problem for developers (since the way I see it are the most afected) and maybe even players, we could say what the ABI is in the "About doomseeker" section, maybe at the left of the version? Like "1.3 (ABI: 3)". That would allow said developer to know very easily if the plugin needs a new version. Quote from Zalewa will also need to be much more rigorous about Doomseeker's ABI No problem. If the API gets on the way, we'll be more careful. Ticket 0001961 will do so (I have not been careful at all with ABI compatibility, so my guess is that it will).

(0020528) Zalewa (developer) 2019-04-22 06:24	Displaying the ABI in the about dialog is not a bad idea.

(0020555) Pol M (developer) 2019-04-27 15:28	PR

(0020587) Zalewa (developer) 2019-05-01 09:43	PR merged The last thing that remains to be done here is to display the "perhaps plugin is for a different Doomseeker version" message in the log.

(0020602) Pol M (developer) 2019-05-01 19:49	PR :)

(0020604) Zalewa (developer) 2019-05-02 12:27 edited on: 2019-05-02 12:29	Cleaned up the PR and merged it here. Also, added a follow-up commit that handles the `%1` formatting placeholders that are present in WinAPI messages. It's probable that the word "Plugin" will fit under the %1 so I naively just substitute this. You can produce a message that normally has this placeholder by creating an empty 'libsomething.dll' file in the "engines/" directory. In any case, the gist of the ticket is fulfilled and I don't see any other problem with current implementation so I consider it as resolved. I'll push the changelog update in a bulk commit after my own work is done.

This issue is already marked as resolved. If you feel that is not the case, please reopen it and explain why.
Supporters:	No one explicitly supports this issue yet.
Opponents:	No one explicitly opposes this issue yet.

Issue History
Date Modified	Username	Field	Change
2018-03-05 22:59	Zalewa	New Issue
2018-10-05 07:22	WubTheCaptain	Priority	normal => low
2019-03-31 15:43	Pol M	Assigned To	=> Pol M
2019-03-31 15:43	Pol M	Status	new => assigned
2019-03-31 15:46	Pol M	Note Added: 0020461
2019-03-31 15:59	Pol M	Note Edited: 0020461	View Revisions
2019-03-31 15:59	Pol M	Status	assigned => needs testing
2019-04-03 11:07	Zalewa	Note Added: 0020470
2019-04-03 13:14	Pol M	Note Added: 0020471
2019-04-03 13:14	Pol M	Status	needs testing => assigned
2019-04-16 18:38	Pol M	Note Added: 0020496
2019-04-16 18:39	Pol M	Status	assigned => needs testing
2019-04-20 06:55	Zalewa	Note Added: 0020514
2019-04-20 06:55	Zalewa	Note Edited: 0020514	View Revisions
2019-04-20 06:56	Zalewa	Note Edited: 0020514	View Revisions
2019-04-20 07:26	Zalewa	Note Added: 0020515
2019-04-20 07:27	Zalewa	Status	needs testing => feedback
2019-04-20 11:53	Pol M	Note Added: 0020517
2019-04-21 13:27	Zalewa	Note Added: 0020520
2019-04-21 13:27	Zalewa	Status	feedback => assigned
2019-04-21 16:05	Pol M	Note Added: 0020524
2019-04-22 06:24	Zalewa	Note Added: 0020528
2019-04-27 15:28	Pol M	Note Added: 0020555
2019-04-27 15:28	Pol M	Status	assigned => needs review
2019-05-01 09:43	Zalewa	Note Added: 0020587
2019-05-01 09:43	Zalewa	Status	needs review => assigned
2019-05-01 17:01	WubTheCaptain	Target Version	=> 1.3
2019-05-01 17:04	WubTheCaptain	Relationship added	parent of 0003644
2019-05-01 19:49	Pol M	Note Added: 0020602
2019-05-02 09:11	WubTheCaptain	Status	assigned => needs review
2019-05-02 12:27	Zalewa	Note Added: 0020604
2019-05-02 12:29	Zalewa	Note Edited: 0020604	View Revisions
2019-05-20 15:37	Pol M	Status	needs review => needs testing
2019-06-23 09:43	Zalewa	Status	needs testing => resolved
2019-06-23 09:43	Zalewa	Fixed in Version	=> 1.3
2019-06-23 09:43	Zalewa	Resolution	open => fixed
2019-07-30 10:13	WubTheCaptain	Status	resolved => closed