Disable printStructure() 2nd try

[D4N]

Re-submission of #165.

The testsuite should fail now due to the missing output of printStructure() & #134 popped up again.

We should also consider either removing printStructure or marking it deprecated, so that downstream code does not use it.

[piponazo]

You mentioned that the test failure is expected. What is the plan then ? Do we need to adjust the test suite ?

[D4N]

Yes, we need to do that as the output of exiv2 is changed quite substantially by the removal of printStructure.

However, as the new python test suite will also require a rewrite of the test output files, we can consider merging this in a broken state and only adapt the python test suite (and abandon the old one). However, before we do that #134 must be fixed again in this patch & the python test suite should be ready.

[D4N]

Now that the Python test suite is merged, we should start working on this. I am afraid that the first step would be to port all regression tests from the old suite to the new one, then remove printStructure and then adapt the new test suite (which will be a ton easier than with the old one). And of course fix #134 along the way.

[D4N]

I just checked the Travis log and there are further issues. The reproducers for #134, #138, #140 result in the following warning: *** Error in '/home/travis/build/Exiv2/exiv2/build/bin/exiv2': free(): corrupted unsorted chunks: 0xsome_hex_number ***. That might the result of a segfault or something even worse (like a subtle heap corruption).

[clanmills]

Gentlemen:

I know what's the matter here. Half a line of code! In tiffimage.cpp#199 readMetadata(), I haven't allocated sufficient memory for the ICC profile. The code:

            iccProfile_.alloc(pos->count());

should be:

            iccProfile_.alloc(pos->count()*pos->typeSize());

I attach a patch which contains the removal of calls to printStructure() for several image types. printStructureFixes.txt

When you apply this patch, you will get lots of changes to the output of the test suite. However it should run without crashes or exceptions. To fix the test suite:

$ make bugfixes
$ cp test/tmp/bugfixes-test.out test/data/

And submit that the modified test/data/bugfixes-test.out

[D4N]

I have pushed your changes Robin.

However I don't think we should update the old test suite any more. It will only further increase the size of the repository (which is already very large).

[clanmills]

Thanks for your work on the test harness and printStructure(). If you're happy that the new test harness embodies the functionality of test/bugfixes-test.sh then I'm happy to remove it.

My understand (from a conversation between you and Luis) is that the binary file test/data/bugfixes-test.out is the culprit. There's another big file: exiv2-bug922a.jpg Is it possible to kill those files and all references so they are no longer in the depot?

A few years ago, Daniel (Kaneider) complained about some EPS and VIDEO test files, so I put them into a different depot and only download them when required. It possible to download a subpart of the depot with git, or do you always have to copy the whole she-bang? We could put those files on the buildserver and pull them down with curl or something.

[D4N]

Yes, the large binary files are the problem. However removing them will not actually solve the problem of the repository size, as the files will be still present in the repos history (they must be present so that you can checkout commits from the past). So not touching them is actually the best approach ;-) (We can of course remove them via git rm).

We were already discussing whether we should move the binary files to another repository running git lfs, but I think we deferred the idea.

[piponazo]

Hi guys. I see this PR referenced in few issues and the one that will solve many of them. Can we make a realistic plan to translate the old tests to use the new python framework so that we can finally merge this?

[piponazo]

Oh ... I just saw #250 . I think this is what we need to solve to finally merge this PR, right ?

[D4N]

Yes, that is correct. However to get this merged, we only need to get the tests from bugfixes-test.sh ported, the other ones are "nice to have" and can be done at some point in the future.

[piponazo]

ey @D4N , what do you think about the following approach:

1. Disable the run of the old bash tests temporarily (bugfixes-test.sh)
2. Merge this PR
3. Port the tests from the old suite to the new suite progressively

In that way we could get fix many of the bug-reports that are being reported around this piece of code. I discussed this briefly with @clanmills and he also agreed.

[D4N]

I don't think that's a good idea, as then there will be even less incentive/motivation to port the test suite, since this PR is then gone. Currently it reminds us that we should take care of it.

[piponazo]

You're right, we will lose motivation to do it. For the moment I have created a new label, so that we can mark all the issues related with the printStructure function until we finish this work.

[clanmills]

I strongly feel we should do this without further delay. I feel very upset every time I see a bug related to printStructure(). Sure, I'm to blame. The punishment of seeing these issue reports is painful.

I have test/bugfixes-test.sh in the discussion document for May. http://dev.exiv2.org/news/3

I believe we can pull bugfixes-test.sh v0.26 from SVN on demand. So we don't need to port everything in bugfixes-test.sh to the new test suite. After we've discussed this, I'll undertake the tasks we agree to complete bugfixes-test for v0.27.

[D4N]

Very well, I am going to drop the commits removing printStructure completely and make a new branch for that, so that we can proceed forward.

[clanmills]

Are you upset? We can talk on Skype if you like.

[D4N]

No, I am not. I agree that removing printStructure completely is too much for now. Let's get the CI green and this thing merged.

[piponazo]

Outstanding work Dan!

I took the freedom to check what was wrong with some of the CI jobs and I fixed few bugs which arose after removing the printStructure calls.