PR addresses observations of Issue #2594.
PR corrects observations of Trac #9690.

This seems to be a duplicate of #2594

It seems unfortunate to make our own tweaks in tinyxml.

I suggest that we update to v7.1.0 or wait

FWIW, tinyxml2 v8.0.0 is currently in the Gentoo tree (but marked as "unstable" - that may change soon). I was trying to head off the mess where people build w/ external tinyxml (Fedora, Gentoo, and others) will start flooding bug reports with XML problems.

The current build practice by some Linux distributions is to patch existing cppcheck code to build against external tinyxml. If they use v8.0.0 and not incorporate PR#811., TestRunner will definitely fail due to node indentation differences. There may be other issues using cppcheck as well.

By providing patched v8.0.0, I'm trying to head off the unnecessary noise from downstream in our direction by giving a fall back until tinyxml2 maintainer incorporates the PR (and releases an update).

ok so it would be pointless to update to v7.1.0 people will get the same problems anyway.

It feels wrong to put this hack in Cppcheck anyway;

• it just does not belong here as far as I see.
• Don't they think it's important that this test fails? If they will link their tinyxml v8 then the test will fail right? Or Cppcheck will not output the proper output?

@scottfurry What is the problem. TinyXML 8.0 indentation is broken and there are redundant spaces or missing spaces? That is not a real problem right? Then I guess our test could be adapted so it removes all indentation and accepts tinyxml output no matter if it's indented properly or not.

Problem occurs w/ v8.0.0. From my read of the issue, a regression was introduced in the update to v8.0.0. This produces missing indentation of nodes. Either way (removing indents or apply PR 811), code patching would be involved. The PR is rather clean and straight forward. From my perspective, it would be more maintainable than "hacking" every usage of XMLPrinter usage and mangling strings for indentation. We rely on "well-formed" xml. I believe we should not change that.

Either way (removing indents or apply PR 811), code patching would be involved

Well patching test/testerrorlogger.cpp seems much better than patching tinyxml.

We rely on "well-formed" xml. I believe we should not change that.

You only patch "our" tinyxml in this PR ... so if gentoo/etc link cppcheck against official tinyxml then the xml will not be well-formed and this test will fail. I don't see what difference it makes for them.

so well.. the choice is;

• if we do nothing then there will be some problems downstream, they can fix by using internal tinyxml.
• we can update tinyxml and patch it. this will lead to problems downstream, they can fix by using internal tinyxml.
• we can update tinyxml and our testerrorlogger, and sacrifice cppcheck output and let the indentation be wrong for a while to save problems downstream

I don't care a lot about the xml indentation. It feels wrong that we check it explicitly in testerrorlogger. But well sacrificing the output to save some problem downstream.. I just think that sounds really weird. Is this really recommended according to Gentoo policy etc?

so well.. the choice is;
*if we do nothing then there will be some problems downstream, they can fix by using internal tinyxml.

Downstream will make noise about build problems, and cause grief for you/project, when things don't work. Doing nothing makes unnecessary work and grief for others.

...
* we can update tinyxml and patch it. this will lead to problems downstream, they can fix by using internal tinyxml.

Update tinyxml and patch to mitigate a regression in tinyxml code. This does not appear to cause a problem in our usage(TestRunner passes completely). When, downstream has problems building, the recommended solution would be to modify their build instructions to use internal. (simple change of a command line flag).

...
* we can update tinyxml and our testerrorlogger, and sacrifice cppcheck output and let the indentation be wrong for a while to save problems downstream

This seems messy to me. We know there is a clear solution and can easily incorporate that fix until next point release upstream at the same time stay current with overall release.

...
Is this really recommended according to Gentoo policy etc?

I'm trying to approach this from a platform agnostic viewpoint and more of a "what is best for cppcheck" standpoint.

Sorry.. I don't understand why choice 1 and 2 are so completely different for downstream.

I have the feeling that with choice 1 and choice 2 the downstream will have the exact same problems.. and will have to make the exact same fix. If they try to link with the shared library for tinyxml 8.0 then they must change their command line to compile with internal.

Does the problems or fix differ somehow?

Bottom Line is that someone will be patching (downstream and/or us) because of the tinyxml2 regression problem with the latest revision. Doing nothing and waiting for a fix would not be a good choice, IMHO.

If we are going to patch, preference would be to minimize impact to cppcheck code because of dependency problem. If downstream has a problem building cppcheck, we can give a workaround. We would be making a mistake in expecting or depending on downstream to patch their version of tinyxml2.

My preference would be to upgrade and patch tinyxml2 (choice #2 from your original list above), something we know will be replaced later on with a tinyxml2 revision increase. The alternative is to "hack" cppcheck code to make work with something buggy(choice #3 above) and rip out later when the problem goes away(tinyxml2 version change).

Trying to minimize the impact of a library problem on the project.

Does the problems or fix differ somehow?

I don't see the answer.

If they compile with internal tinyxml then it will work. No matter if we take choice 1 or 2.

If they try to compile with shared library tinyxml then it will not work. No matter if we take choice 1 or 2. The fix will be to compile with internal tinyxml.

bottomline is; I don't think it's elegant to have a patched tinyxml in our repo. it's an ugly choice that I do not feel comfortable with.

It is not critical that testerrorlogger checks the indentation. That is a "side effect" in the test. There is a choice #4 that we do not update our tinyxml at all. We write a ASSERT_EQUAL_XML that ignores indentation and use that in testerrorlogger. Well if gentoo wants to link with the shared library then, they can do it. indentation from cppcheck will not be perfect but they probably have such problem for other packages also.

ok