[PATCH 0/1] MR6735: msxml3: Undo removal of xmlThrDefTreeIndentString
Fixes regression by 9d34c44d8464722c333fdde5313b341504d3ea44. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57325 Not sure why this was removed, but it breaks the office installer. If this is not the right course of action, please tell me and I'll try to find a different solution. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735
From: Fabian Maurer <dark.shadow4(a)web.de> Fixes regression by 9d34c44d8464722c333fdde5313b341504d3ea44. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=57325 --- dlls/msxml3/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dlls/msxml3/main.c b/dlls/msxml3/main.c index c92cd49aaf3..43cbf664ca5 100644 --- a/dlls/msxml3/main.c +++ b/dlls/msxml3/main.c @@ -365,8 +365,10 @@ BOOL WINAPI DllMain(HINSTANCE hInstDLL, DWORD fdwReason, LPVOID reserved) case DLL_PROCESS_ATTACH: xmlInitParser(); - /* Set the default indent character to a single tab */ + /* Set the default indent character to a single tab, + for this thread and as default for new threads */ xmlTreeIndentString = "\t"; + xmlThrDefTreeIndentString("\t"); /* Register callbacks for loading XML files */ if(xmlRegisterInputCallbacks(wineXmlMatchCallback, wineXmlOpenCallback, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6735
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149315 Your paranoid android. === debian11b (64 bit WoW report) === mf: mf.c:6609: Test failed: Unexpected hr 0x102. Report validation errors: winmm:mci crashed (c0000008)
The function is marked as deprecated in recent versions, so presumably we are supposed to use something else? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86126
It was deprecated in https://gitlab.gnome.org/GNOME/libxml2/-/commit/75b5bc2b0a6219ee03069611793d..., with no MR or other explanation. Maybe the expected replacement is setting xmlTreeIndentString in every thread? We could do that, we have a DllMain right there, but that'd initialize libxml state even for unrelated threads. Feels like there should be a better solution somewhere... but I don't know where. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86134
On Fri Oct 25 21:18:34 2024 +0000, Alexandre Julliard wrote:
The function is marked as deprecated in recent versions, so presumably we are supposed to use something else? The function sets a global variable, and that has a comment to use `xmlSaveSetIndentString`.
Problem 1: This function doesn't exist in the current version Problem 2: We need to set in in `domdoc.c:doparse`, it is later used inside `xmlsave.c:xmlNodeDumpOutput`, but this function makes its own save context, and we can't configure that one... In short, I currently don't see a better way than setting it globally with the deprecated function. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86135
On Fri Oct 25 21:18:34 2024 +0000, Fabian Maurer wrote:
The function sets a global variable, and that has a comment to use `xmlSaveSetIndentString`. Problem 1: This function doesn't exist in the current version Problem 2: We need to set in in `domdoc.c:doparse`, it is later used inside `xmlsave.c:xmlNodeDumpOutput`, but this function makes its own save context, and we can't configure that one... In short, I currently don't see a better way than setting it globally with the deprecated function. If I'm reading the source code correctly, that is almost correct. Yes, the function makes a save context - but first, it checks if there is one, and if yes, it keeps that.
If you're sure that everything calling xmlNodeDumpOutput goes via doparse, you can create and configure a save context in doparse. The context is created as soon as it's referenced; simply write to xmlTreeIndentString, it's a macro expanding to a function call that allocates if needed. Then you don't need the write in DllMain anymore. (Yes, it's horrifying.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86136
On Fri Oct 25 21:31:04 2024 +0000, Alfred Agrell wrote:
If I'm reading the source code correctly, that is almost correct. Yes, the function makes a save context - but first, it checks if there is one, and if yes, it keeps that. If you're sure that everything calling xmlNodeDumpOutput goes via doparse, you can create and configure a save context in doparse. The context is created as soon as it's referenced; simply write to xmlTreeIndentString, it's a macro expanding to a function call that allocates if needed. Then you don't need the write in DllMain anymore. (Yes, it's horrifying.) I guess setting `xmlTreeIndentString = "\t";` works, but I'm not sure if that's the only places where it's need.
Also, not sure what you mean by "create and configure a save context in doparse", it is automatically setup in xmlNodeDumpOutput, and setting xmlTreeIndentString is enough for that to work. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86145
On Fri Oct 25 21:44:09 2024 +0000, Fabian Maurer wrote:
I guess setting `xmlTreeIndentString = "\t";` works, but I'm not sure if that's the only places where it's need. Also, not sure what you mean by "create and configure a save context in doparse", it is automatically setup in xmlNodeDumpOutput, and setting xmlTreeIndentString is enough for that to work. You're right, I mixed up the structs. I meant the global state, not the save context.
And yes, it needs to be set somewhere. I don't think doparse is correct, though; I think the correct places are domdoc_save and domdoc_get_xml. Hopefully that list won't grow. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86147
On Fri Oct 25 22:42:38 2024 +0000, Alfred Agrell wrote:
You're right, I mixed up the structs. I meant the global state, not the save context. And yes, it needs to be set somewhere. I don't think doparse is correct, though; I think the correct places are domdoc_save and domdoc_get_xml. Hopefully that list won't grow. To make sure we'd need to do it for all "normal" saves where we can use `xmlSaveSetIndentString` and then all places that call `xmlNodeDumpOutput` directly and indirectly.
That could be a bunch of places, including `node_get_xml`, in our office case. This could easily lead to a regression if we miss one place. Do we want to go that route? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86148
On Fri Oct 25 22:59:56 2024 +0000, Fabian Maurer wrote:
To make sure we'd need to do it for all "normal" saves where we can use `xmlSaveSetIndentString` and then all places that call `xmlNodeDumpOutput` directly and indirectly. That could be a bunch of places, including `node_get_xml`, in our office case. This could easily lead to a regression if we miss one place. Do we want to go that route? Looks like the recommended replacement is not xmlTreeIndentString - that one will be deprecated as well in libxml >= 2.14. The recommended replacement is configuring the save context, using a function that doesn't exist until 2.14. https://gitlab.gnome.org/GNOME/libxml2/-/commit/35146ff31cfc4698050e4500b267...
For now, this MR looks like a good approach to me. The function is not deprecated in libxml2 2.12.8, we can ignore the issue until the next libxml2 upgrade. (Also looks like I suck at reading libxml2 source code. xmlSaveSetIndentString has been deprecated for five months, not two years.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86163
(Also looks like I suck at reading libxml2 source code. xmlSaveSetIndentString has been deprecated for five months, not two years.)
xmlSaveSetIndentString is not deprecated, did you mean something else?
The recommended replacement is configuring the save context, using a function that doesn't exist until 2.14
But that won't work for the `xmlNodeDumpOutput` case, should we report that usecase to them? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86184
On Sat Oct 26 21:45:08 2024 +0000, Fabian Maurer wrote:
(Also looks like I suck at reading libxml2 source code. xmlSaveSetIndentString has been deprecated for five months, not two years.) xmlSaveSetIndentString is not deprecated, did you mean something else? The recommended replacement is configuring the save context, using a function that doesn't exist until 2.14 But that won't work for the `xmlNodeDumpOutput` case, should we report that usecase to them? xmlSaveSetIndentString is not deprecated in libxml 2.12.8 (the one we're currently using), but it is deprecated in current libxml master. https://gitlab.gnome.org/GNOME/libxml2/-/commit/592546267fdde8ed57be99e168be...
Looks like you could call xmlSaveTree instead of xmlNodeDumpOutput, but that is a little more typing. Feel free to ask about it if you want a second opinion. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86185
On Sat Oct 26 22:21:59 2024 +0000, Alfred Agrell wrote:
xmlSaveSetIndentString is not deprecated in libxml 2.12.8 (the one we're currently using), but it is deprecated in current libxml master. https://gitlab.gnome.org/GNOME/libxml2/-/commit/592546267fdde8ed57be99e168be... Looks like you could call xmlSaveTree instead of xmlNodeDumpOutput, but that is a little more typing. Feel free to ask about it if you want a second opinion. No, `xmlSaveSetIndentString` is not deprecated in current master, it doesn't occur in the commit you linked either.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86186
On Sat Oct 26 22:26:09 2024 +0000, Fabian Maurer wrote:
No, `xmlSaveSetIndentString` is not deprecated in current master, it doesn't occur in the commit you linked either. ...blah, I keep mixing things up. I linked the xmlThrDefTreeIndentString deprecation, not xmlSaveSetIndentString. The latter is indeed not deprecated - it's the recommended replacement for the relevant deprecations.
And I mixed up xmlSaveSetIndentString with xmlTreeIndentString in my previous post. It's the latter that's been deprecated for five months. I should stop posting here, I'm contributing more noise than signal. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_86187
Alexandre, any news on this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_87061
What we can do here: - patch our libxml2 copy to set xmlTreeIndentString to tab by default (is that what we need?); - remove use of deprecated symbols, including xmlThrDefTreeIndentString symbol; - merge that; - once 2.14 is out, we'll switch to save context API everywhere with xmlSaveSetIndentString(); - xmlNodeDumpOutput will have to be reimplemented locally, unless libxml2 people are willing to add an alternative that takes a save context; -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_87481
On Thu Nov 14 18:48:32 2024 +0000, Nikolay Sivov wrote:
What we can do here: - patch our libxml2 copy to set xmlTreeIndentString to tab by default (is that what we need?); - remove use of deprecated symbols, including xmlThrDefTreeIndentString symbol; - merge that; - once 2.14 is out, we'll switch to save context API everywhere with xmlSaveSetIndentString(); - xmlNodeDumpOutput will have to be reimplemented locally, unless libxml2 people are willing to add an alternative that takes a save context; Why is patching the copy better than using a deprecated symbol? You want to undo both anyways.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_87511
Let's use xmlThrDefTreeIndentString() if it works. Could you please add a small test, so we don't break it again? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_88249
@ovarley1 already has a test for this: https://gitlab.winehq.org/ovarley1/wine/-/commit/98e8712ae9891e0efb44caada1e... Perhaps this MR could go in and then Orin can create an MR for the test? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_88250
On Thu Nov 21 09:33:49 2024 +0000, Huw Davies wrote:
@ovarley1 already has a test for this: https://gitlab.winehq.org/ovarley1/wine/-/commit/98e8712ae9891e0efb44caada1e... Perhaps this MR could go in and then Orin can create an MR for the test? Sure. We'll need a change for this one to include <libxml/xmlsave.h> anyway, otherwise you'll get a compilation warning. That's why there is no CI results.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_88260
[msxml.diff](/uploads/d080564bc8d59722b7f68af3cbf73dc9/msxml.diff) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6735#note_88261
participants (7)
-
Alexandre Julliard (＠julliard) -
Alfred Agrell (＠Alcaro)
-
Fabian Maurer
-
Fabian Maurer (＠DarkShadow44)
-
Huw Davies (＠huw)
-
Marvin -
Nikolay Sivov (＠nsivov)