Make autotools work again

[kyz]

t-w's work on ADFLib has been amazing, implementing a lot of new functionality, adding tests, and greatly improving the the build with CMake and on Windows compilers

However, this left a number of deficiencies in the autotools build. This PR fixes them.

1. The autotools build did not support all standard autotools features, including out-of-tree build from a read-only source directory
2. The autotools build did not package all the files needed in the distribution tarball
3. The autotools build installed all the header files for the library in /usr/include including several include files that aren't prefixed "adf". This wasn't noticed because the Debian build script, which is what was really being checked, chose to install them in /usr/include/adf from the source tree, not letting autotools do the install.
4. There were a lot of configure.ac tests that aren't used. Tests in configure.ac are only needed if you then consume the results of the test. There's no point checking if a header file exists if you don't then wrap usage of the header in the source code with #if HAVE_HEADERNAME_H
5. The libtool version info that defines the shared library version should be updated independently of the package version number. They are not related, and to try and tie them together either really restricts your package version numbering system - or what is more common is that you completely forget about the library version numbering and pick a nice looking package version number that appears on all the websites, etc., that breaks the technical requirements of the library versioning system. This PR introduces a util/bump-version script that can be told what type of change you've made (no library changes, backward-compatible library changes, breaking library changes) and will compute the next version number for both the package and the library version.
6. The check library, used for the unit tests, was a hard dependency. You couldn't even configure the package without it being installed. This is now changed so that if you don't have the library, the unit tests won't be compiled or run, but the library will still build and install and the regression tests will still build and run.

The previous version of this PR also made optional the code that has been added to unadf to mess up the filenames so they'll be usable on a Win32 system (totally not needed on UNIX), but I think I'll let this PR focus on the build system, and put the Win32 filename mangling fix in another PR

[t-w]

Unfortunately "Make UNIX work again" is "make Debian Stable not work anymore"... ;-) The current version of autoconf in Debian Stable (11) is 2.69. So for me the change of the required autoconf version to 2.71 is breaking... Debian 12 is on the way but until then (couple of months I guess), I would like to keep compatibility with current Debian Stable, which is like a reference Linux version for me. I think the priority in the context of a library should be rather possibly wide compatibility/portability, not necessarily everything super new and shiny, esp. concerning the build systems.

As for lack of the files required by autotools - they were removed in 26a0f9bcb, so not by me. I put them back in my fork a while ago...

Concerning removing replace_not_allowed_chars() - while maybe it is not true that those characters are "not allowed" (only '/' and '\0' are really forbidden, probably the same with Amiga...), but they are highly unsafe as they may interfere with the shell. So, what I would propose is:

1. keep that removed function for "not windows" and rename it to replace_unsafe_chars() (for clarity)
2. either
   • keep it as default and add a switch to disable it (I vote for this as it is more safe for users, IMHO)
   • or disable it by default and enable with a switch I think it would be safer for users to leave this - at least as an option (if not as default).

I have a pretty big update coming up which will be in conflict in many of changes done here. I haven't made a pull or a merge since a while as first I needed to have things reasonably working. Soon (few days maybe) I should have something more-less ready so I will create a PR for a review (if you want a peek at it check t-w/ADFlib/tree/devel ). I would like to ask for eventually closing this PR for now to give me a bit of time, and apply these changes later - if that would be ok for you. That would also allow us to clarify the things discussed above. (otherwise, I can merge this and resolve conflicts, but it will delay me again, so...)

[kyz]

I'm more than happy to wait for your update to come first and make this change later.

Sorry if I implied your changes broke the autotools build, I did not look closely at the change history.

Also I'm OK with winding the autoconf requirement back to 2.69 (2.71 was written by autoupdate but I'm not sure it requires >2.69)

My main concern is that unadf used to unpack files with their original Amiga filenames directly to Linux/UNIX filesystems, which like the Amiga FS can accept any character in filenames except "/" and null, and the recent changes mean now it applies transforms that are really only needed for Win32 filesystems.. To me, it doesn't make sense to force renaming to meet Win32 naming conventions on a Linux system.

Personally I use unadf on my Linux system to unpack ADFs in the same way one would use unzip to unpack archives... I unpack to directories that are used as virtual harddisks in UAE, and I expect it to work like I mounted the ADF in UAE and copied the files across inside the emulator.

Perhaps we can meet in the middle? I could make this transformation code always part of unadf (not compilation flag dependent), and instead controlled by a command-line switch. Win32 systems could default to being it turned on, Linux default to it turned off.

What do you think?

[lclevy]

I'm thinking about using adflib as a python module to directly write into zip archive using zipfile module.

[lclevy]

With python and Qt, we can have a portable directory opus to browse adf content using a gui. I have ideas, but no time to contribute, sorry 😐

[michaelortmann]

This patch helps with some issues, but is still failing on arch linux during make:

/bin/sh ../libtool  --tag=CC   --mode=link gcc  -g -O2   -o unadf unadf.o ../src/libadf.la  
libtool: link: gcc -g -O2 -o .libs/unadf unadf.o  ../src/.libs/libadf.so -Llinux -ladfnative -Wl,-rpath -Wl,/usr/local/lib
/usr/bin/ld: cannot find -ladfnative: No such file or directory
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:418: unadf] Error 1
make[2]: Leaving directory '/home/michael/usr/src/ADFlib/examples'
make[1]: *** [Makefile:467: all-recursive] Error 1
make[1]: Leaving directory '/home/michael/usr/src/ADFlib'
make: *** [Makefile:376: all] Error 2

toolchain:

autoconf (GNU Autoconf) 2.71
GNU Make 4.4.1
libtool (GNU libtool) 2.4.7.4-1ec8f-dirty
gcc version 12.2.1 20230201 (GCC)
GNU ld (GNU Binutils) 2.40

there is a file: src/linux/libadfnative.a but its not picked up?

[t-w]

@kyz, yes, that would be fine I think - to keep the function for UNIXes, but enable it only in a "safe" extraction mode enabled by a switch (and of course - the function for Windows with larger restrictions enabled by default, or, if necessary, maybe even mandatory - with no way of disabling it).

[kyz]

I will rebase this and add in all the distcheck fixes from https://github.com/lclevy/ADFlib/pull/34 and I'll check what it does on arch linux

[t-w]

Some change here breaks autotools build on CygWin:

$ make
make  all-recursive
make[1]: Entering directory '/cygdrive/d/src/adflib/lclevy/ADFlib'
Making all in src
make[2]: Entering directory '/cygdrive/d/src/adflib/lclevy/ADFlib/src'
  CC       adf_dev.lo
In file included from adf_dev.c:31:
adf_dev.h:42:6: warning: ISO C99 doesn't support unnamed structs/unions [-Wpedantic]
   42 |     };
      |      ^
adf_dev.c:297:9: error: conflicting types for 'adfReadBlockDev'; have 'long int(struct AdfDevice * const,  const uint32_t,  const uint32_t,  uint8_t * const)' {aka 'long int(struct AdfDevice * const,  const unsigned int,  const unsigned int,  unsigned char * const)'}
  297 | RETCODE adfReadBlockDev ( struct AdfDevice * const dev,
      |         ^~~~~~~~~~~~~~~
In file included from adf_dev.c:31:
adf_dev.h:59:9: note: previous declaration of 'adfReadBlockDev' with type 'RETCODE(struct AdfDevice * const,  const uint32_t,  const uint32_t,  uint8_t * const)' {aka 'int(struct AdfDevice * const,  const unsigned int,  const unsigned int,  unsigned char * const)'}
   59 | RETCODE adfReadBlockDev ( struct AdfDevice * const dev,
      |         ^~~~~~~~~~~~~~~
adf_dev.c:315:9: error: conflicting types for 'adfWriteBlockDev'; have 'long int(struct AdfDevice * const,  const uint32_t,  const uint32_t,  const uint8_t * const)' {aka 'long int(struct AdfDevice * const,  const unsigned int,  const unsigned int,  const unsigned char * const)'}
  315 | RETCODE adfWriteBlockDev ( struct AdfDevice * const dev,
      |         ^~~~~~~~~~~~~~~~
In file included from adf_dev.c:31:
adf_dev.h:64:9: note: previous declaration of 'adfWriteBlockDev' with type 'RETCODE(struct AdfDevice * const,  const uint32_t,  const uint32_t,  const uint8_t * const)' {aka 'int(struct AdfDevice * const,  const unsigned int,  const unsigned int,  const unsigned char * const)'}
   64 | RETCODE adfWriteBlockDev ( struct AdfDevice * const dev,
      |         ^~~~~~~~~~~~~~~~
make[2]: *** [Makefile:545: adf_dev.lo] Error 1
make[2]: Leaving directory '/cygdrive/d/src/adflib/lclevy/ADFlib/src'
make[1]: *** [Makefile:465: all-recursive] Error 1
make[1]: Leaving directory '/cygdrive/d/src/adflib/lclevy/ADFlib'
make: *** [Makefile:374: all] Error 2

The same source tree builds fine under the same CygWin but with CMake. So something is wrong with these autotools changes.

I tried to pinpoint what causes this but so far I do not see what exactly happens. It might be something with includes (paths) and typedefs/defines (apparently RETCODE has different type in different files, so some header is messing this up). But for now, I am not sure what is the exact source of the problem here...

[t-w]

Actually, I have just found the guilty - src/win32/adf_nativ.h defines RETCODE as:

#define RETCODE long

while in adf_def.h, it is:

typedef int32_t RETCODE;

hence the type conflict...

So this is to fix.

[kyz]

It depends on the version of autotools - on current Debian stable (11/bullseye) it fails (automake 1.16.3), passes on CygWin where I have automake 1.16.5.

As we discussed somewhere before, increasing requirements on version components of the build system is completely unnecessary and does not improve usability, but limits the number of systems supported.

It was introduced in 1.16.4

Common top-level files can be provided as .md; the non-md version is used if both are present: AUTHORS ChangeLog INSTALL NEWS README README-alpha THANKS

It seems like a useful feature to have. Why on earth would we need an empty README next to a non-empty README.md ?

What I can do instead is initialise the foreign flavour of autotools, as the default flavour is gnu (strict). This also removes the need for an empty NEWS file.

[kyz]

Actually, I have just found the guilty - src/win32/adf_nativ.h defines RETCODE as:

#define RETCODE long

while in adf_def.h, it is:

typedef int32_t RETCODE;

hence the type conflict...

So this is to fix.

I am very happy to fix this. The adf_nativ.h files have this odd dependency where they're not just a header for the native driver function, they are also defining what RETCODE is for that type of system.

I'm happy to make it int32_t throughout.

[t-w]

Actually, I have just found the guilty - src/win32/adf_nativ.h defines RETCODE as:

#define RETCODE long

while in adf_def.h, it is:

typedef int32_t RETCODE;

hence the type conflict... So this is to fix.

I am very happy to fix this. The adf_nativ.h files have this odd dependency where they're not just a header for the native driver function, they are also defining what RETCODE is for that type of system.

I'm happy to make it int32_t throughout.

Those native device headers are also redefining BOOL. I was wondering why wasn't it using just what is in adf_defs.h. maybe there was an idea to make the native devce module completely independent, having types depending on the OS... I do not know. Anyway, we can clean these up, I think, as does not seem to have much sense (and is making trouble).

[t-w]

It depends on the version of autotools - on current Debian stable (11/bullseye) it fails (automake 1.16.3), passes on CygWin where I have automake 1.16.5. As we discussed somewhere before, increasing requirements on version components of the build system is completely unnecessary and does not improve usability, but limits the number of systems supported.

It was introduced in 1.16.4

Common top-level files can be provided as .md; the non-md version is used if both are present: AUTHORS ChangeLog INSTALL NEWS README README-alpha THANKS

So here we are....

It seems like a useful feature to have. Why on earth would we need an empty README next to a non-empty README.md ?

Because... it is required by automake 1.16.3? Which is something available in a stable version of one of the most important Linux distros?

It does not have to be empty, we can put there a brief description, just redirection to another file or whatever. I do not care. I only care that it works. And without it, it doesn't...

What I can do instead is initialise the foreign flavour of autotools, as the default flavour is gnu (strict). This also removes the need for an empty NEWS file.

If we are sure that it does not break anything again... As you see, even removing a seemingly unimportant file has consequences, so let's not change all the places, now.

Really, there is a lot to do with the lib, and we are wasting time on not really significant details (which can be done, OK., but as part of polishing, not the major thing). Let't not complicate simple things, just leave the file for now, with whatever contents, and forget it.

More important would be eg. updating the doc, or the main readme file...

There are more places to clean-up, but let's start from necessary things.

[kyz]

it is required by automake 1.16.3?

Well not quite. automake 1.16.3 lacks the useful functionality of recognising that README.md is equivalent to README, in reflection of how many people are writing their readme files in markdown, but more generally, this is not an official GNU project. If it were, it would built on whatever the maintainer(s) chose, including choosing automake 1.16.4 (which would make sense if they also chose to supply README in markdown), and made into a distribution tarball. From that point on, it would not matter what the version of autotools was, because they're not needed by the end user.

The README file is required by the default gnu strictness, which makes checks for conventions we are not following.

I like what the autotools mythbuster has to say, it was very persuasive:

Most of your projects are likely to use [foreign], even though that is not the default, because it relaxes some checks that are, otherwise, often worked around in tutorials. [...]

The two main differences between the gnu and foreign flavours is that the former requires the presence of a number of files in the top-level of the projects, such as NEWS, COPYING, AUTHORS, ChangeLog, README. Often enough, at least the first file in this list is just touch-ed to stop automake from failing.

Note: Even if you plan on using these files the way GNU does, it is still recommended to use the foreign flavour, and manually list these files in Makefile.am so that they are actually installed in the correct place; the gnu flavour only requires them to be distributed, not to be actually installed.

If we are applying workarounds of empty files to appease the default gnu flavour, we are probably doing it wrong. We should select foreign flavour, because that is what we are, and not apply workarounds.

[t-w]

it is required by automake 1.16.3?

Well not quite. automake 1.16.3 lacks the useful functionality of recognising that README.md is equivalent to README, in reflection of how many people are writing their readme files in markdown, but more generally, this is not an official GNU project. If it were, it would built on whatever the maintainer(s) chose, including choosing automake 1.16.4 (which would make sense if they also chose to supply README in markdown), and made into a distribution tarball. From that point on, it would not matter what the version of autotools was, because they're not needed by the end user.

The README file is required by the default gnu strictness, which makes checks for conventions we are not following.

I like what the autotools mythbuster has to say, it was very persuasive:

Most of your projects are likely to use [foreign], even though that is not the default, because it relaxes some checks that are, otherwise, often worked around in tutorials. [...] The two main differences between the gnu and foreign flavours is that the former requires the presence of a number of files in the top-level of the projects, such as NEWS, COPYING, AUTHORS, ChangeLog, README. Often enough, at least the first file in this list is just touch-ed to stop automake from failing. Note: Even if you plan on using these files the way GNU does, it is still recommended to use the foreign flavour, and manually list these files in Makefile.am so that they are actually installed in the correct place; the gnu flavour only requires them to be distributed, not to be actually installed.

If we are applying workarounds of empty files to appease the default gnu flavour, we are probably doing it wrong. We should select foreign flavour, because that is what we are, and not apply workarounds.

ADFlib contains all the required files, except text version of README and NEWS. I took mc as a random example - its ChangeLog contains:

For the major changes since the last release please see doc/NEWS. For
the detailed commit log please refer to the output of 'git log' against
a checked out copy of the repository.

Are they making a workaround this way?

Why cannot we just put "See ChangeLog" in NEWS and "See README.md" in README and forget about it?

Changing flavor to foreign disables "portability warnings" (info from the link you provided) - so it is not just not checking for required info files.

[kyz]

Are they making a workaround this way?

Yes they are. We should either follow the GNU standards or not. We don't follow them - there is no summary of changes per release in a file called NEWS. There is an empty NEWS file, put there just to placate an automated check. That's not reeally following the standards. That's placating an automated check while not following the standards.

The tool doing the check also gives you an option to turn off the check. As we are not really following the standards, why not use that option?

Changing flavor to foreign disables "portability warnings" (info from the link you provided) - so it is not just not checking for required info files.

I've just read through the source code of automake, and I can tell you it's still checking for a lot of files, and will ensure they're there at any level. These files are required at GNU level strictness, but not FOREIGN level strictness:

• ABOUT-NLSif you have a po directory
• AUTHORS, ChangeLog, INSTALL, NEWS, README (or same with .md suffix since 1.16.3)
• One of COPYING, COPYING.LIB, COPYING.LESSER

That's the full extent of the difference. All other required files, all other checks are still present. The automake documentation is overly vague.

[kyz]

I've added the unadf.1 manpage from Debian and updated it... also now the actions are running, I can see where things are broken, right in the PR. I've made fixes for those too.

[t-w]

Are they making a workaround this way?

Yes they are. We should either follow the GNU standards or not.

This is a totalitarian approach in Anakin's style with 'If you are not with me you are my enemy." - that I completely do not agree with. If packages that are way more widely used do not follow it 100%, why do we suddenly have to? And why put so much energy into it?

I start to understand why the state of the lib has not improved in some years, if the discussions were focused on not important details instead of eg. proper read support (which was practically working for only one case - sequential reading, which worked only for simple cases like the extraction tool unadf). And nobody noticed that functionally, for client code, most of the file support was either not working at all or buggy.

We don't follow them - there is no summary of changes per release in a file called NEWS. There is an empty NEWS file, put there just to placate an automated check. That's not reeally following the standards. That's placating an automated check while not following the standards.

We do not follow them in 2 tiny places which do not matter, and we are wasting time discussing it.

There is also a plethora of places and aspects where the lib does not follow proper coding standards, which matter way more for the quality and security of the library, contrary to the discussed, which does not. It is like focusing on one tiny dandelion while your garden is full of other weed. It just makes no sense.

The tool doing the check also gives you an option to turn off the check. As we are not really following the standards, why not use that option?

Because of what you write below - it will stop checking for presence of COPYING. Which, contrary to overrides in NEWS and README, matters.

Changing flavor to foreign disables "portability warnings" (info from the link you provided) - so it is not just not checking for required info files.

I've just read through the source code of automake, and I can tell you it's still checking for a lot of files, and will ensure they're there at any level. These files are required at GNU level strictness, but not FOREIGN level strictness:

* `ABOUT-NLS`if you have a `po` directory

* `AUTHORS`, `ChangeLog`, `INSTALL`, `NEWS`, `README` (or same with `.md` suffix since 1.16.3)

* One of `COPYING`, `COPYING.LIB`, `COPYING.LESSER`

That's the full extent of the difference. All other required files, all other checks are still present. The automake documentation is overly vague.

And this is doing more harm than good. COPYING is very important is should be checked. Otherwise some smartass may delete it and do with the code whatever he wants saying it is a PD, not GNU.

To conclude this, because I am really losing my patience for this - for me, NEWS and README must be kept, in whatever form. Period.

@lclevy should have final decision for it. And that is what I wait for, I am not discussing this any further.

[kyz]

why do we suddenly have to?

We don't, we never did, that's very much the point of foreign, to flag to automake we use that we don't,

It removes 8 unnecessary -f tests, and you appear to be saying that's the end of the world.

You're insistent that, while we don't follow the GNU standards, we must leave automake on the default setting, which requires that we do follow them... but not really. We'll put in blank files, whatever, to pass a check that a file exists, but we won't actually put contents in that file. But you also can't accept turning the check off, even though it's of no value.

Otherwise some smartass may delete it and do with the code whatever he wants saying it is a PD, not GNU.

This is not how copyright works.

Are you seriously arguing that the enforcement of licenses is left to an optional program that only maintainers run, where it errors out if a named file is missing but doesn't check its contents (echo This is in the public domain >COPYING), and even the check can be turned off with a supported and recommended command-line argument?

for me, NEWS and README must be kept

I'll do it to placate you personally, out of respect for your many other contributions. I do not agree with your specific argument here.

[t-w]

@lclevy, @kyz - I have updated AUTHORS and ChangeLog (see the commits above). Please, have a look and, if needed, complete/correct.

Before merging this branch, we have to test it for all systems.

I have started testing this on Debian stable, after we get this working I will check also on CygWin (which was working with both autotools and CMake).

At the moment, on Debian stable configure goes like this:

$ ./configure 
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking whether make supports nested variables... (cached) yes
checking build system type... x86_64-pc-linux-gnu
checking host system type... x86_64-pc-linux-gnu
Host OS: linux-gnu
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables... 
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking whether gcc understands -c and -o together... yes
checking whether make supports the include directive... yes (GNU style)
checking dependency style of gcc... gcc3
checking for ar... ar
checking the archiver (ar) interface... ar
checking how to print strings... printf
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 1572864
checking how to convert x86_64-pc-linux-gnu file names to x86_64-pc-linux-gnu format... func_convert_file_noop
checking how to convert x86_64-pc-linux-gnu file names to toolchain format... func_convert_file_noop
checking for /usr/bin/ld option to reload object files... -r
checking for objdump... objdump
checking how to recognize dependent libraries... pass_all
checking for dlltool... no
checking how to associate runtime and link libraries... printf %s\n
checking for archiver @FILE support... @
checking for strip... strip
checking for ranlib... ranlib
checking command to parse /usr/bin/nm -B output from gcc object... ok
checking for sysroot... no
checking for a working dd... /bin/dd
checking how to truncate binary pipes... /bin/dd bs=4096 count=1
checking for mt... mt
checking if mt is a manifest tool... no
checking how to run the C preprocessor... gcc -E
checking for ANSI C header files... yes
checking for sys/types.h... yes
checking for sys/stat.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for memory.h... yes
checking for strings.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for unistd.h... yes
checking for dlfcn.h... yes
checking for objdir... .libs
checking if gcc supports -fno-rtti -fno-exceptions... no
checking for gcc option to produce PIC... -fPIC -DPIC
checking if gcc PIC flag -fPIC -DPIC works... yes
checking if gcc static flag -static works... yes
checking if gcc supports -c -o file.o... yes
checking if gcc supports -c -o file.o... (cached) yes
checking whether the gcc linker (/usr/bin/ld -m elf_x86_64) supports shared libraries... yes
checking whether -lc should be explicitly linked in... no
checking dynamic linker characteristics... GNU/Linux ld.so
checking how to hardcode library paths into programs... immediate
checking whether stripping libraries is possible... yes
checking if libtool supports shared libraries... yes
checking whether to build shared libraries... yes
checking whether to build static libraries... yes
checking for pkg-config... /usr/bin/pkg-config
checking pkg-config is at least version 0.9.0... yes
checking for check >= 0.9.6... yes
checking for int32_t... yes
checking for size_t... yes
checking for uint16_t... yes
checking for uint32_t... yes
checking for uint8_t... yes
./configure: line 12698: 0.7.13=0.7.13: command not found
./configure: line 12700: 0:12:0=0:12:0: command not found
./configure: line 12702: 2022-01-25=2022-01-25: command not found
checking that generated files are newer than configure... done
configure: creating ./config.status
config.status: creating Makefile
config.status: creating src/Makefile
config.status: creating doc/Makefile
config.status: creating examples/Makefile
config.status: creating regtests/Test/Makefile
config.status: creating tests/Makefile
config.status: creating adflib.pc
config.status: creating config.h
config.status: executing depfiles commands
config.status: executing libtool commands

after which make ends with:

[...]
  CC       adf_vol.lo
In file included from ../../../src/adf_vol.c:36:
../../../src/adf_dev.h:42:6: warning: ISO C99 doesn’t support unnamed structs/unions [-Wpedantic]
   42 |     };
      |      ^
In file included from ../../../src/adf_str.h:31,
                 from ../../../src/adf_vol.h:31,
                 from ../../../src/adf_vol.c:32:
../../../src/adf_vol.c: In function ‘adfCreateVol’:
../../../src/adf_defs.h:63:1: warning: ISO C forbids braced-groups within expressions [-Wpedantic]
   63 | ({                           \
      | ^
../../../src/adf_vol.c:283:21: note: in expansion of macro ‘min’
  283 |     unsigned nlen = min ( (unsigned) MAXNAMELEN,
      |                     ^~~
  CC       linux/adf_nativ.lo
In file included from ../../../src/adf_nativ.h:27,
                 from ../../../src/linux/adf_nativ.c:36:
../../../src/adf_dev.h:42:6: warning: ISO C99 doesn’t support unnamed structs/unions [-Wpedantic]
   42 |     };
      |      ^
  CCLD     libadf.la
Usage: /home/user/src/github/adflib/lclevy/autotools/ADFlib/build/autotools/libtool [OPTION]... [MODE-ARG]...
Try 'libtool --help' for more information.
libtool:   error: you must specify an output file
make[2]: *** [Makefile:494: libadf.la] Error 1
make[2]: Leaving directory '/home/user/src/github/adflib/lclevy/autotools/ADFlib/build/autotools/src'
make[1]: *** [Makefile:465: all-recursive] Error 1
make[1]: Leaving directory '/home/user/src/github/adflib/lclevy/autotools/ADFlib/build/autotools'
make: *** [Makefile:374: all] Error 2

---

Trying to build debs, I am getting:

/bin/bash ../libtool  --tag=CC   --mode=link gcc -std=c99 -pedantic -Wall -Wextra -Werror-implicit-function-declaration  -g -O2 -ffile-prefix-map=/home/user/src/github/adflib/lclevy/deb/ADFlib=. -fstack-protector-strong -Wformat -Werror=format-security -version-info  -Wl,-z,relro -o libadf.la -rpath /usr/lib/x86_64-linux-gnu adf_dev.lo adf_dev_flop.lo adf_dev_hd.lo adf_raw.lo adf_bitm.lo adf_dump.lo adf_util.lo adf_env.lo adf_dir.lo adf_file.lo adf_file_block.lo adf_cache.lo adf_link.lo adf_salv.lo adf_vol.lo generic/adf_nativ.lo    
libtool:   error: CURRENT '-Wl,-z,relro' must be a nonnegative integer
libtool:   error: '-Wl,-z,relro' is not valid version information
make[3]: *** [Makefile:494: libadf.la] Error 1
make[3]: Leaving directory '/home/user/src/github/adflib/lclevy/deb/ADFlib/src'
make[2]: *** [Makefile:465: all-recursive] Error 1
make[2]: Leaving directory '/home/user/src/github/adflib/lclevy/deb/ADFlib'
make[1]: *** [Makefile:374: all] Error 2
make[1]: Leaving directory '/home/user/src/github/adflib/lclevy/deb/ADFlib'
dh_auto_build: error: make -j4 returned exit code 2
make: *** [debian/rules:20: build] Error 25
dpkg-buildpackage: error: debian/rules build subprocess returned exit status 2
debuild: fatal error at line 1182:
dpkg-buildpackage -us -uc -ui failed

so something is wrong with updates in autotools config (CMake builds still work).

This has to be resolved, after that I will test this also on CygWin (it was working with both autotools and CMake).

Also, @kyz do you want the autotools to work also on MacOs? A while ago, when I was trying this, it was failing (I do not remember on what). That's why the Actions config builds only with CMake (and only shared libs, building a static lib does not work, I am not sure why, either...). If you want, we can eventually add a config to build with autotools for MacOs, but I might not be able to help too much with fixing autotools for that... (If you do not want it, we leave it, for now at least).

[kyz]

./configure: line 12698: 0.7.13=0.7.13: command not found
./configure: line 12700: 0:12:0=0:12:0: command not found
./configure: line 12702: 2022-01-25=2022-01-25: command not found

libtool:   error: you must specify an output file

/bin/bash ../libtool  --tag=CC   --mode=link [,,,] -version-info  -Wl,-z,relro  [...]

Substitutions are missing. I will look into why

Also, @kyz do you want the autotools to work also on MacOs?

I will take a look

[t-w]

1. The ad727cb is not make autotools work again, this is new stuff, I want this thing finished and merged esp. that it is broken, now. So for me - new PR/issue.

2. Would it be possible to put these new tests under tests (like tests/examples)? We have already tests in 2 places, that would be the 3rd... I am thinking about moving the regtests under tests also - but not now

Smaller steps, things are supposed to be improved, not more broken. Can we finish this PR first, then start adding new things? It is discussed in #40 what are the priorities for now.

Btw. I will add Ubuntu LTS to the Actions - it should help testing with older version of software (and Ubuntu LTS, like Debian stable, must definitely be supported; they should be similar, so it may help).

[kyz]

1. The ad727cb is not make autotools work again, this is new stuff, I want this thing finished and merged esp. that it is broken, now. So for me - new PR/issue.

I already fixed it while you were writing your comment.

It turns out that you run tests/test_examples outwith the build, as well as in the CMake / autotools test suites. I now know this, and correct for this.

Also, this test_examples really didn't test much. So actual coverage of the example programs was almost zero. Now it's a little higher, because the output of the commands is actually checked. All part of a good build system!

I wanted to write a test for unadf, to test the new -w option, and also wanted to move the test to where the code is (examples), so I intended to do it here. It's a good thing too, it already caught two bugs in unadf (which are fixed directly in master), and also revealed that unadf when given specific filenames to extract, does not set their permissions (because it doesn't look up their directory entry). Something to fix later and then test for.

2. Would it be possible to put these new tests under tests (like tests/examples)? We have already tests in 2 places, that would be the 3rd... I am thinking about moving the regtests under tests also - but not now

Sure, this is a matter of taste, and if you want to move all the tests under one folder, that's possible. But generally each "subsystem" gets its own tests to match, and we have two subsystems: "examples" and "src".

The other part, which is why I won't move these examples tests under tests/, is because now all the tests in tests require the check library, and no tests outwith tests require it, fits the conditional build of tests depending on whether the check library is installed or not. If all tests are in one directory, it changes from "conditional subdir" to "conditional test objects" which is a lot more work.

Btw. I will add Ubuntu LTS to the Actions - it should help testing with older version of software (and Ubuntu LTS, like Debian stable, must definitely be supported; they should be similar, so it may help).

That sounds fine. Could you also add running these test examples to the cmake builds? They are already tested as part of the normal build, from the build tree, but you also want to test the installed commands, and I see the non-existent script util/test_examples is commented out. The new test-all-examples.sh might work, but I haven't had time to try

[kyz]

I see you've opened a new PR for adding ubuntu LTS. I was expecting it in this branch.

Is there anything else you want to see in this branch, such as adding an action for macos autotools build? Other than that, I'd consider this PR completed, and if you want, I can just merge this anyway and look into macos on a different PR?

[t-w]

I see you've opened a new PR for adding ubuntu LTS. I was expecting it in this branch.

I am testing things first in my fork. Good practice in general, not to pollute the main repo. For now is build on LTS failing (on unit tests?), I need to resolve it, first.

Is there anything else you want to see in this branch, such as adding an action for macos autotools build? Other than that, I'd consider this PR completed, and if you want, I can just merge this anyway and look into macos on a different PR?

Not really. MacOs, as I wrote is up to you. If ever, can be done later, in a separated PR or whatever. It can be build with CMake. At some point, it would be nice to have.

But first, I would like to have things that where until now working - working. So until Debian stable and CygWin is not tested, we are not merging. There is too many breaking changes here to merge this untested.

Btw. after this, we freeze the changes with build systems to absolute minimum, as changes in this is a huge waste of time as have to be tested everywhere and usually are breaking something. Supporting more configs/OSs is a priority, while changes, if not necessary, can be done later.

Such changes have to be tested everywhere (not only in Actions - this is very limited... good that it is at all) and then can be merged.

[t-w]

OK. so in the end, I am not adding Ubuntu LTS... It is 20.04 and it has some completely ancient check (0.10.0-3build2), packages with Check in Ubuntu: https://packages.ubuntu.com/search?keywords=check&searchon=names&suite=all&section=all

In fact the "latest" Ubuntu in actions is 22.04 (so the newer LTS...), and this one has the same version of check as Debian stable. So adding the other one makes no sense...

[kyz]

If the difference is just the package version of the check library, then please update the configure,ac, here, with the earliest version of the check library that works.

PKG_CHECK_MODULES([CHECK], [check >= 0.9.6], [tests=yes], [tests=no])

Presumably if 0.10 doesn't work and 0.15 does, then the version you actually need is in that range.

[t-w]

If the difference is just the package version of the check library, then please update the configure,ac, here, with the earliest version of the check library that works.

PKG_CHECK_MODULES([CHECK], [check >= 0.9.6], [tests=yes], [tests=no])

Presumably if 0.10 doesn't work and 0.15 does, then the version you actually need is in that range.

Well, basically the first test does not compile...

make[1]: Leaving directory '/home/runner/work/ADFlib/ADFlib/regtests/Test'
Making check in tests
make[1]: Entering directory '/home/runner/work/ADFlib/ADFlib/tests'
make  test_adfDays2Date test_adfPos2DataBlock test_adf_file_util test_file_append test_file_create test_file_overwrite test_file_overwrite2 test_file_seek test_file_seek_after_write test_file_truncate test_file_truncate2 test_file_write test_file_write_chunks test_test_util
make[2]: Entering directory '/home/runner/work/ADFlib/ADFlib/tests'
gcc -DHAVE_CONFIG_H -I. -I..  -std=c99 -Wall -Wextra -pedantic -Werror-implicit-function-declaration -I../src -I../src/linux  -pthread -g -O2 -MT test_adfDays2Date-test_adfDays2Date.o -MD -MP -MF .deps/test_adfDays2Date-test_adfDays2Date.Tpo -c -o test_adfDays2Date-test_adfDays2Date.o `test -f 'test_adfDays2Date.c' || echo './'`test_adfDays2Date.c
test_adfDays2Date.c: In function ‘test_adfDays2Date’:
test_adfDays2Date.c:58:1: warning: ISO C forbids nested functions [-Wpedantic]
   58 | Suite * adflib_suite ( void )
      | ^~~~~
test_adfDays2Date.c:74:1: warning: ISO C forbids nested functions [-Wpedantic]
   74 | int main ( void )
      | ^~~
test_adfDays2Date.c:74:5: warning: ‘main’ is normally a non-static function [-Wmain]
   74 | int main ( void )
      |     ^~~~
make[2]: Leaving directory '/home/runner/work/ADFlib/ADFlib/tests'
test_adfDays2Date.c:85:1: error: expected declaration or statement at end of input
   85 | }
      | ^
At top level:
test_adfDays2Date.c:74:5: warning: ‘main’ defined but not used [-Wunused-function]
   74 | int main ( void )
      |     ^~~~
make[2]: *** [Makefile:881: test_adfDays2Date-test_adfDays2Date.o] Error 1
make[1]: *** [Makefile:1541: check-am] Error 2
make: *** [Makefile:470: check-recursive] Error 1
make[1]: Leaving directory '/home/runner/work/ADFlib/ADFlib/tests'
Error: Process completed with exit code 2.

as would probably the others. So they changed something in the API(???). Probably we will just update the required revision, there is not much gain in adding this.

Also, I thought that ubuntu-latest in Actions is the latest. It is just the latest LTS... So we leave it there.

OK. I will check Debian and CygWin and let you know.

[kyz]

What I notice is that END_TEST is blank in check 0.15, and your tests don't have END_TEST at the end of them, so perhaps it does work on check 0.10, provided you add END_TEST in the right places.

https://github.com/libcheck/check/blob/master/NEWS

Sat Oct 20, 2019: Released Check 0.13.0

• END_TEST is now optional, as how START_TEST works has been redone

You require check >= 0.13.0

[t-w]

What I notice is that END_TEST is blank in check 0.15, and your tests don't have END_TEST at the end of them, so perhaps it does work on check 0.10, provided you add END_TEST in the right places.

You are correct. I have missed it... I will fix the tests.

https://github.com/libcheck/check/blob/master/NEWS

Sat Oct 20, 2019: Released Check 0.13.0

• END_TEST is now optional, as how START_TEST works has been redone

You require check >= 0.13.0

I will see if corrected tests work. If, they are, we can, eventually (maybe optionally) enable build of that old LTS. It seems that, in the end, there will be not much gain in adding this (I would rather see some eg. BSD instead...).

Thanks for checking this!

[t-w]

Building with CygWin, there is some issue with devices:

[...]
In file included from ../../../src/adf_str.h:31,
                 from ../../../src/adf_vol.h:31,
                 from ../../../src/adf_vol.c:32:
../../../src/adf_vol.c: In function 'adfCreateVol':
../../../src/adf_defs.h:63:1: warning: ISO C forbids braced-groups within expressions [-Wpedantic]
   63 | ({                           \
      | ^
../../../src/adf_vol.c:283:21: note: in expansion of macro 'min'
  283 |     unsigned nlen = min ( (unsigned) MAXNAMELEN,
      |                     ^~~
  CC       win32/adf_nativ.lo
In file included from ../../../src/adf_nativ.h:27,
                 from ../../../src/win32/adf_nativ.c:36:
../../../src/adf_dev.h:42:6: warning: ISO C99 doesn't support unnamed structs/unions [-Wpedantic]
   42 |     };
      |      ^
../../../src/win32/adf_nativ.c: In function 'Win32InitDevice':
../../../src/win32/adf_nativ.c:76:13: error: implicit declaration of function 'Win32ReleaseDevice' [-Werror=implicit-function-declaration]
   76 |             Win32ReleaseDevice ( dev );
      |             ^~~~~~~~~~~~~~~~~~
../../../src/win32/adf_nativ.c:46:59: warning: unused parameter 'ro' [-Wunused-parameter]
   46 |                                  const BOOL               ro )
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~^~
../../../src/win32/adf_nativ.c: At top level:
../../../src/win32/adf_nativ.c:137:16: error: static declaration of 'Win32ReleaseDevice' follows non-static declaration
  137 | static RETCODE Win32ReleaseDevice ( struct AdfDevice * const dev )
      |                ^~~~~~~~~~~~~~~~~~
../../../src/win32/adf_nativ.c:76:13: note: previous implicit declaration of 'Win32ReleaseDevice' with type 'int()'
   76 |             Win32ReleaseDevice ( dev );
      |             ^~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[2]: *** [Makefile:552: win32/adf_nativ.lo] Error 1
make[2]: Leaving directory '/cygdrive/d/src/adflib/lclevy/autotools/ADFlib/build/autotools/src'
make[1]: *** [Makefile:465: all-recursive] Error 1
make[1]: Leaving directory '/cygdrive/d/src/adflib/lclevy/autotools/ADFlib/build/autotools'
make: *** [Makefile:374: all] Error 2

Something with headers? I will check also VS with native win32 (by default only generic is build with CMake).

Edit: it builds with VS(?).

The problem is that with the removal/merging the adf_nativ.h. Now, the adf_nativ.c lacks prototypes of the local/static functions. Some are called inside, not only externally.

I have this open so I will fix it.

[t-w]

OK. If the checks passes, I think we are OK. So go ahead and merge it.

I will check with fuseadf with this and we can see what else to eventually fix for 0.8.0 (#40). And finally tag it.

Ah. there is that fix for adfFileOpen ("w" to some const). I will do it after merging this.

[lclevy]

I'm happy to see things are converging. Thanks guys!

[t-w]

Why haven't you merged it? I see you started adding new features. But we were at the point things were tested. Some manually. as there is no infra in GH to do it. Now something can be broken, eg. Debian, again...

We have to work in some steps, do some bunch, test and merge.

It is good you want add more tests to support more system's - but this can break existing ones, which cannot be tested automatically in GH (unless we add custom runners, not likely...). So let's merge this with the tested things. Then open a new PR ("adding cygwin and mingw"), with new features.

We cannot have everything at once and I really want to close 0.8.0 (#40). Not spend next half a year adding features before doing it.

We had a request from a Debian maintainer for a new version, the last one 0.7.12 is ancient and, as reported, not compiling anymore. We postponed it a long time anyway, as there was crucial functionality to fix or add. More things - we do for next minor/major rev.

So, please, merge what was tested and open a new PR for the things you are adding (move it to another branch).

[kyz]

I've tried adding autotools builds for macos (homebrew) and windows (cygwin and msys2)

• macos build works
  • macos found that there is not always a gstdbuf command. Removed its usage
• cygwin works
  • but it is very slow to install, can take 17 minutes, so currently disabled
  • distcheck found that src/win32/nt4_dev.h was missing as a dependency of win32/adf_nativ.o
  • msys2 doesn't currently work
    • failing in examples tests. when comparing the output of commands vs heredoc in script, one of the two has CRLF line endings and the other doesn't, causing diffs to fail. one of the two will need to be switched to LF
    • also, when printing paths, "/tmp" changes somewhere to "D:/a/_temp/msys64/tmp", can probably be fixed by changing tmpdir if cd $tmpdir && pwd does not print the same value as $tmpdir

[t-w]

Good. But as I wrote - these are new features. Another branch/PR.

Each such thing is time consuming. So as I wrote before, smaller steps.

[kyz]

Revealing what's broken when tested on a wider range of environments is not a "new feature"

"Debian again"? -> https://github.com/lclevy/ADFlib/actions/workflows/deb-build.yml?query=is%3Afailure

[t-w]

Revealing what's broken when tested on a wider range of environments is not a "new feature"

Yes, it is. New feature of the build system.

"Debian again"? -> https://github.com/lclevy/ADFlib/actions/workflows/deb-build.yml?query=is%3Afailure

What do you mean by this?

[t-w]

Make this a new PR. This one is closed. #49