Add an option to build translation tables by ddeclerck · Pull Request #220 · OCamlPro/gnucobol

ddeclerck · 2025-03-10T17:39:26Z

This PR adds a -gentable option to GnuCOBOL that allows to build EBCDIC/ASCII translation tables using iconv.
It outputs the table to stdout - it is expected that users will redirect that to an acutal file.

GitMensch

overall that is already a good implementation, I think the missing pieces would be done quickly; note that --gentable should likely be referenced in gnucobol.texi where the translation tables are documented

cobc/help.c

config/ebcdic500_ascii8bit.ttbl

cobc/gentable.c

build_windows/vs2010/cobc.vcxproj

cobc/gentable.c

GitMensch · 2025-03-25T12:49:59Z

.github/workflows/windows-msys2.yml

+          #../configure --without-db --without-curses --without-xml2 --without-json \
+          #             --without-iconv --disable-dependency-tracking
          ../configure --without-db --without-curses --without-xml2 --without-json \
-                       --without-iconv --disable-dependency-tracking
+                       --disable-dependency-tracking


we shouldn't adjust the CI config - as noted above this is only for generating the intermediate tarball and that must work without iconv as well

Yeah, but as far as I remember, something was failing (don't remember what). I'll revert this change so we can check that.

See, for MSYS2, we have undefined references to libiconv in gentable.c - although all these calls are guarded by a HAVE_ICONV ifdef. And this only fails on MSYS2...

EDIT: apparently under MSYS2 configure insists on setting HAVE_ICONV 1 in config.h...

I guess this means that it does have iconv in glibc - which symbols are undefined during link of cobc?

The undefined symbols are 'libiconv', 'libiconv_open' and 'libiconv_close'.
See this CI run.

Would you have any clue about this one ?

It seems that there's something broken in configure.ac, I'll try to have a look at this later.

Found it, the iconv stuff is included by gettext and because of the AC_REQUIRE those parts are all done even before the conditions are run. But I've found a solution by setting the iconv cache variable to "no", in case of explicit disabled iconv (which then can break gettext, but that should be explained by configure).

fixed upstream, should work fine now on MSYS2, both --with-iconv and --without-iconv

build_windows/ocide/cobc.exe.cpj

cobc/gentable.c

GitMensch · 2025-03-25T13:06:29Z

config/ebcdic500_ascii8bit.ttbl

ebcdic500_latin1.ttbl should still be adjusted to not include code but the reference to --gentable, also it should be able to recreate that exact table using --gentable, correct? If the output is somehow better formatted/commented, then it should be adjusted in that file as if it would be generated.

also it should be able to recreate that exact table using --gentable, correct?

Yes it is. Except that in the "manual" file we named the encodings EBCDIC 500 and Latin-1, which are not recognized by iconv - we'd need to pass IBM500,ISO-8859-1 to generate this table.

If the output is somehow better formatted/commented, then it should be adjusted in that file as if it would be generated.

Appart from the issue above, and a license text added to the "manual" file, the tables are formatted identically.

ddeclerck · 2025-03-25T14:28:37Z

Note the unrelated MacOS failure - this has started to fail when they deployed a new MacOS image...
Don't know exactly what is going on ; the build step fails with No rule to make target '/config.status', needed by 'Makefile', although the file seems to have been created correctly in the previous step. The leading / is suspicious though, so maybe some kind of path issue...

EDIT: apparently Makevars is not correctly inserted in po/Makefile.in.in - maybe something related to the version of gettext...

GitMensch

nearly done: the new option should be added to NEWS as new cobc option with a very short note what to use it for - you are free to directly do that in the upstream commit (note my minor changes to gentable.c); also build_windows and cobc and gnucobol.texi should get an update (possibly the Changelogs and the files touched need an update of the copyright year to 2025 as well)

... and a testcase be added (maybe just create the ebcdic500_ascii8bit.ttbl reversible/non-reversible, of course only if iconv is available)

GitMensch

minor change requests; note: the reversible option is definitely a good one

NEWS

cobc/help.c

tests/testsuite.src/used_binaries.at

cobc/gentable.c

doc/gnucobol.texi

GitMensch

good for upstream

* allow early exit if iconv opening does not work * don't check an unsigned short for < 0

GitMensch · 2025-03-28T15:58:35Z

The test 31 should really be adjusted so it takes both the "expected" amount of line breaks and the unexpected ones, if we don't see a way to fix presumably flush not happening...

Interesting msvc debug failures!

GitMensch

I think everything here is fine now... as soon as the new tests pass the MSVC debug builds.

ddeclerck · 2025-03-29T20:31:41Z

I think everything here is fine now... as soon as the new tests pass the MSVC debug builds.

If only...
For some reason, in debug builds, the iconv call on untranslatable characters does not set errno to EILSEQ - see the output I added to gentable: errno is 0 after the call (while res is = -1). Could that be an iconv bug ?

GitMensch · 2025-03-30T15:21:02Z

So currently we always get ret==-1 in all cases we expect and get errno set; but with MSVC debug builds we have -1 and errno not set. That does neither match the Linux manpage nor the posix definition.

I'm tempted to say: revert the errno output, replacing it with a comment to the strange behaviour, and adjust the config script to not enable iconv in case of debug builds, then consider this finished and merge upstream.

Possibly retry later, depending on the state of vcpkg bug microsoft/vcpkg#44698.

Thoughts?

ddeclerck · 2025-03-31T13:52:04Z

So currently we always get ret==-1 in all cases we expect and get errno set; but with MSVC debug builds we have -1 and errno not set. That does neither match the Linux manpage nor the posix definition.

I'm tempted to say: revert the errno output, replacing it with a comment to the strange behaviour, and adjust the config script to not enable iconv in case of debug builds, then consider this finished and merge upstream.

Possibly retry later, depending on the state of vcpkg bug microsoft/vcpkg#44698.

Thoughts?

I found a comment online (see last comment here) that this could be caused by a mix of debug and release DLLs. But I don't know, really.

I guess disabling iconv for MSVC debug builds is okay for now.

GitMensch · 2025-03-31T15:42:21Z

LGTM

ddeclerck · 2025-03-31T20:46:03Z

Merged @ SVN 5470.

GitMensch · 2025-03-31T20:47:00Z

Well done. So up to the dependency fixup next?

ddeclerck · 2025-03-31T20:47:29Z

Well done. So up to the dependency fixup next?

Yes.

GitMensch · 2025-04-04T20:15:23Z

Just to let you know (as the CI builds work again thanks to your test 31 adjustment): old MinGW fails with the iconv conversion and therefore the one testcase is broken:

error: conversion from IBM500 to ASCII is not supported by your iconv implementation

it would be possible to check the TESTMODE and if it is set don't exit with EXIT_FAILURE but 77 (skip tests)
What do you think?

ddeclerck · 2025-04-05T15:15:53Z

Just to let you know (as the CI builds work again thanks to your test 31 adjustment): old MinGW fails with the iconv conversion and therefore the one testcase is broken:

error: conversion from IBM500 to ASCII is not supported by your iconv implementation

it would be possible to check the TESTMODE and if it is set don't exit with EXIT_FAILURE but 77 (skip tests) What do you think?

That would be possible.

Alternatively, we could skip the test if the command iconv -f IBM500 -t ASCII fails (assuming both libiconv and iconv are installed).

P.S: which MinGW workflow are you using ? The one we have here does not even have iconv, hence the test is just skipped.

GitMensch · 2025-04-06T06:40:39Z

The binary and library are independent, especially as the iconv functions may be part of another library, including libc. Can you please implement the test within cobc to adjust the return code in this case?

This
Is is the CI definition used (the MinGW part depends on a tarball created by Ubuntu in another CI).

ddeclerck · 2025-04-07T08:38:52Z

Actually one part of the test is for checking the return code & error message when requesting an unsupported translation:

AT_CHECK([$COBC --gentable=EBCDIC_XXX,ASCII_XXX], [1], [],
[error: conversion from EBCDIC_XXX to ASCII_XXX is not supported by your iconv implementation
])

Just returning 77 in testmode would skip the whole test.
If we want to keep that check for unsupported conversions for the general case, then maybe we can skip the test specifically when the IBM500/ASCII conversion is not supported, for instance with something like:

AT_SKIP_IF([test "$($COBC --gentable=IBM500,ASCII 2>&1)" = "error: conversion from IBM500 to ASCII is not supported by your iconv implementation"])

GitMensch · 2025-04-07T10:15:10Z

You're right, that's a good idea in general.

I've also thought about the option that this code may just be different named, but after checking with the binary artifact - neither ibm500 nor cp500 is supported (ASCII is).

ddeclerck · 2025-04-07T10:33:01Z

You're right, that's a good idea in general.

I've also thought about the option that this code may just be different named, but after checking with the binary artifact - neither ibm500 nor cp500 is supported (ASCII is).

Yeah, I was wondering about that too ; it just seems these encodings are not supported by old versions of libiconv.

GitMensch · 2025-04-07T11:21:51Z

I'm checking that one-liner in, no need to rebase this.

ddeclerck force-pushed the gen_tables branch 4 times, most recently from a2b5ed2 to 5bd9321 Compare March 11, 2025 08:42

GitMensch requested changes Mar 24, 2025

View reviewed changes

GitMensch requested changes Mar 25, 2025

View reviewed changes

ddeclerck force-pushed the gen_tables branch from f43f05f to 948239a Compare March 25, 2025 14:23

GitMensch requested changes Mar 25, 2025

View reviewed changes

GitMensch requested changes Mar 26, 2025

View reviewed changes

ddeclerck force-pushed the gen_tables branch from ac1add8 to 9465e96 Compare March 27, 2025 16:42

GitMensch approved these changes Mar 27, 2025

View reviewed changes

ddeclerck and others added 6 commits March 28, 2025 16:11

Add an option to build translation tables

bd2d69e

Updates after review

2893e2d

minor gentable.c adjustments

fa68c57

* allow early exit if iconv opening does not work * don't check an unsigned short for < 0

More updates

1bbc1aa

More updates

97660e4

More updates

4809fdc

ddeclerck force-pushed the gen_tables branch from e666179 to 4809fdc Compare March 28, 2025 15:37

GitMensch requested changes Mar 29, 2025

View reviewed changes

ddeclerck force-pushed the gen_tables branch from 62d14e4 to fb1afe3 Compare March 29, 2025 20:14

ddeclerck force-pushed the gen_tables branch 2 times, most recently from ec18c58 to a416afb Compare March 30, 2025 11:04

ddeclerck force-pushed the gen_tables branch 3 times, most recently from 517244d to 0645017 Compare March 31, 2025 14:43

ddeclerck force-pushed the gen_tables branch 3 times, most recently from 8be6f0b to 6b973c9 Compare March 31, 2025 15:14

Fixups

dee2372

ddeclerck force-pushed the gen_tables branch from 6b973c9 to dee2372 Compare March 31, 2025 15:16

ddeclerck added Ready for SVN Merged in SVN labels Mar 31, 2025

GitMensch removed the Ready for SVN label Mar 31, 2025

Skip gentable test if IBM500/ASCII not supported

e501750

GitMensch closed this Apr 7, 2025

ddeclerck deleted the gen_tables branch July 29, 2025 07:33

Conversation

ddeclerck commented Mar 10, 2025

Uh oh!

GitMensch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GitMensch Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

ddeclerck Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

ddeclerck Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitMensch Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

ddeclerck Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

ddeclerck Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

GitMensch Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

GitMensch Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GitMensch Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeclerck Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeclerck commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GitMensch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GitMensch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GitMensch left a comment

Choose a reason for hiding this comment

Uh oh!

GitMensch commented Mar 28, 2025

Uh oh!

GitMensch left a comment

Choose a reason for hiding this comment

Uh oh!

ddeclerck commented Mar 29, 2025

ddeclerck Mar 25, 2025 •

edited

Loading

GitMensch Mar 27, 2025 •

edited

Loading

GitMensch Mar 25, 2025 •

edited

Loading

ddeclerck Mar 25, 2025 •

edited

Loading

ddeclerck commented Mar 25, 2025 •

edited

Loading

GitMensch left a comment •

edited

Loading