possible regression in 3.7

[nieder]

When running the tests for enca, I get 2 test failures (see https://github.com/nijel/enca/issues/30), one of them in test-recode.sh.

The tests pass when I have recode-3.6 installed, but fail with recode-3.7.

[rrthomas]

Sorry, I don't have time at present to dig into other code bases. If you can find what the problem (or difference) is with recode, and even better, write a failing test for recode, that would be amazing.

A quick glance suggests that the failure is to do with trying to seek a non-seekable file descriptor (e.g. a pipe). Recode 3.7 removes configurable strategies, and uses pipes if they're available. Without more digging (and paging back into my head work I did some months ago!) I can't say whether this is likely to be a misuse of the Recode API, or whether I've broken it in some way.

[nieder]

I walked test builds through recode history (basically git bisect by hand) and enca-1.19 built against recode commit 4fb3980 fails its test. But if built against recode commit cd53528 which is one commit earlier, then enca passes all its tests.

[rrthomas]

Thanks, I'll have a look to see if that tells me anything.

[nieder]

Unfortunately, my coding ability is zero but I can certainly test patches or similar. Thanks for looking into this.

[rrthomas]

@nieder, thanks very much for your persistence and help with this!

[rrthomas]

I just had a look at these two commits, and a) they were made nine and ten years ago respectively, and not by me; and b) they involve no code changes. So I'm a bit suspicious that they would cause this difference.

I'm sorry, but owing to a continued lack of time, I can't look further into this issue at the moment. If anyone can pinpoint the problem in terms of recode (e.g. "this recode API should do X but it does Y") then I should be able to look into it.

[shlomif]

After some investigation, I found out that recode probably fclose()s a FILE that is passed to it from enca which later expects to use it again. See convert_recode() in enca.

[rrthomas]

Thanks @shlomif. I guess recode shouldn't close file descriptors it is given.

However, there are other weird things about the code in enca; for example, at line 127 of convert_recode.c it tries to restore the original file from the copy if the recoding succeeded (success == true).

I really don't have time to unpick this. If someone can provide a failing test for recode, then I can work on fixing recode.

[seb321]

@rrthomas I don't see such test in master branch. Nevertheless, the problem is that recode_perform_task (task.c) closes given streams. If I comment cleanup out in this function (lines 425-435), enca passes all tests. I don't know if you only need to delete the cleanup or change some code too, but hope it will help.

[rrthomas]

Sorry, I wasn't clear. I meant, if someone can write a new test that fails, I will happily fix the bug.

[seb321]

I meant that there is no conditional test at line 127 of convert_recode.c in master branch of enca.

I understand that you want a failing unit test, but I am afraid that I can't help with it. I do not know python well enough to create one. (If someone wants to create it, then it should only check if streams given to recode_perform_task are still opened after call.)

[rrthomas]

Please could you try with the attempted fix I've just pushed, which does not close a file descriptor in the final step if it was supplied by the caller to perform_task?

[seb321]

Thanks for fix. With it enca passes all tests and it seems that it works correctly.

[nieder]

This is the enca test-suite.log with recode-3.7:

====================================
   Enca 1.19: test/test-suite.log
====================================

# TOTAL: 21
# PASS:  19
# SKIP:  0
# XFAIL: 0
# FAIL:  2
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: ./test-tex.sh
===================

enca: `recode' doesn't look like a valid converter name
enca: Cannot seek in file `/tmp/encaWxedt8': Bad file descriptor
enca: Cannot close file `/tmp/encaWxedt8': Bad file descriptor
enca: `recode' doesn't look like a valid converter name
FAIL test-tex.sh (exit status: 1)

FAIL: ./test-recode.sh
======================

enca: Cannot close file `/tmp/encaauJfTd': Bad file descriptor
enca: Cannot close file `/tmp/encaJY8i5A': Bad file descriptor
enca: Cannot seek in file `test-recode.actual': Bad file descriptor
enca: Cannot close file `test-recode.actual': Bad file descriptor
enca: librecode probably damaged `STDIN'. No way to recover in a pipe.
1d0
< n?por po?ouchl?ch ?lut?ch ???an? p?ehlu?il i zu??c? ork?n
FAIL test-recode.sh (exit status: 1)

This is the enca test-suite.log with recode-3.7 and src/task.c patched with f075752

====================================
   Enca 1.19: test/test-suite.log
====================================

# TOTAL: 21
# PASS:  20
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: ./test-recode.sh
======================

enca: librecode probably damaged `STDIN'. No way to recover in a pipe.
1d0
< n?por po?ouchl?ch ?lut?ch ???an? p?ehlu?il i zu??c? ork?n
FAIL test-recode.sh (exit status: 1)

[seb321]

@nieder Hmm, on my machine enca with fix passes all tests. I will check it one more time.

[seb321]

@nieder I cannot reproduce results you got.

============================================================================
Testsuite summary for Enca 1.19
============================================================================
# TOTAL: 21
# PASS:  21
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

============================================================================
Testsuite summary for Enca 1.20-dev
============================================================================
# TOTAL: 21
# PASS:  21
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

[rrthomas]

Just to be clear: tests of the fix should be against recode master HEAD. I can't see any other changes since 3.7 that might have affected this bug, but equally I can't be sure, and assuming we can sort this fix out, I'll make a new release from master HEAD.

[rrthomas]

Closing, absent any further evidence that the tests are still failing with latest recode.