avrintel.h missing from installation

[drmpjz]

Build environment: Linux openSUSE 15.5

Sorry, not a cmake expert, so I don't have a patch :-(

Problem:
libavrdude.h ist installed in the include directory of CMAKE_INSTALL_PREFIX:PATH (e.g. /usr/include or /usr/local/include)

In libavrdude.h you have since 7.3

#include "avrintel.h"

but this is not installed to the same path, so any compilations of programs using libavrdude.h will fail.

avrdude itself just works, because it is build in the full source tree.

I've found that after the build in build_linux there is a file named install_manifest.txt, which contains a list of the files and path of all the files that final install step will distribute on the system. I expect that avrintel.h just needs to be added there.

Unfortunately I did not find an (to me) obvious place in the build systems to do so.

[mcuee]

It seems to me we need to change the following file.
https://github.com/avrdudes/avrdude/blob/main/src/CMakeLists.txt

Unfortunately I also do not know much about CMake and I do not know how to fix it.

[drmpjz]

Maybe here?

set_target_properties(libavrdude PROPERTIES
    PREFIX ""
    PUBLIC_HEADER "libavrdude.h"
    VERSION 1.0.0
    SOVERSION 1
    )

PUBLIC_HEADER seems to be the keyword. Now this only needs to be a list.

P.S. I have tested that if I copy avrintel.h manually to /usr/include both compilation of the program works and the resulting binary also works as expected.

[drmpjz]

PUBLIC_HEADER "libavrdude.h;avrintel.h"

seems to do the trick (still no cmake expert, only stackoverflow knowledge...)

[mcuee]

@Youw

Sorry to disturb you. Just wondering if the work-around by @drmpjz is correct or not. Thanks.

[ndim]

So it appears that the header part of the API is for libavrdude users to #include <libavrdude.h>, and that libavrdude.h file then unfortunately does #include "avrintel.h" without libavrdude actually installing avrintel.h.

(I have not found any examples for a libavrdude user within the avrdude sources, or any libavrdude users for that matter. The avrdude utility is not easily separable from libavrdude in the current source code tree, with the code for both being mixed inside the same directory src/.)

The addition of #include "avrintel.h" to libavrdude.h is a relatively recent addition from the 2023-11-23 commit fd1c49e by @stefanrueger which also happens to remove #include "avrintel.h" from a number of compilation units which all #include <libavrdude.h>.

That patch adds three function prototypes to libavrdude.h, one of which actually use the Configitem_t type which is defined inside avrintel.h:

const Configitem_t *avr_locate_config(const Configitem_t *cfg, int nc, const char *name);
int avr_get_config_value(const PROGRAMMER *pgm, const AVRPART *p, const char *cname, int *valuep);
int avr_set_config_value(const PROGRAMMER *pgm, const AVRPART *p, const char *cname, int value);

The commit message only mentions the two _set_ and _get_ functions and makes no mention of the avr_locate_config function which actually requires the Configitem_t type and therefore the inclusion of avrintel.h from libavrdude.h.

So adding #include "avrintel.h" and the avr_locate_config function appears like an unintended change to the library API.

The quick workaround is to install avrintel.h, but that will expose a LOT of machine generated AVR part related constants as part of the libavrdude.h API, which looks like a future nightmare to maintain compatibility-wise. Besides, the point of APIs and ABIs is to be small.

The avrintel.h file contains a note that it is generated by mkavrintel.pl which is not part of the avrdude source code repository, and mkavrintel cannot be found on the internet with a search engine either. Perhaps mkavrintel.pl could be included in the avrdude repo and rewritten to split off the typedef for Configitem_t from the avrintel.h with all the parts related constants? Or, if mkavrintel.pl has disappeared and avrintel.h now must be maintained by hand, split off the public types part from a private types part and private constants part?

BTW, I do not see any changes to library symbol versioning or soname or similar corresponding to adding these three functions to the ABI. That might be due to my eyes being unused to cmake after 20+ years of working with automake+libtool in the libgphoto2 buildsystem, but the ABI related definitions inside src/CMakeLists.txt appear to originate from 2022-01-11 commit 8be18c7 by @yegorich and have not been changed since.

If the libavrdude library is intended as a serious way to use the avrdude code, it might be interesting for the future to separate the source code for the libavrdude library and the avrdude utility by keeping it in separate subdirectories, e.g. inc/ for the public library headers, lib/ for the library code and src/ for the utility code. Then avrintel.h would have been inside the lib/ subdir for all its library internal constants, and compiling the avrdude utility would have failed due to it not having access to avrintel.h.

From my experience with libgphoto2 + libgphoto2_port + gphoto2 I would recommend keeping everything one source code repository avrdude with one buildsystem, but separating the library and the utility within that one source code repo's build system (inc/, lib/, src/). Splitting things into different buildsystems and splitting into different repos when all you are doing is coordinated releases causes a lot of extra work for very close to no benefits, so I would refrain from that.

BTW, where is the libavrdude library API and ABI documented, and how are library users supposed to find the library? Just try the default include and linker path? Some modern library allow themselves flexibility for future incompatible API changes by installing their header files into PREFIX/include/foo-1/foo.h. So library users would use -IPREFIX/include/foo-1.0 and then use #include <foo.h>, as discovered by a foo-1.pc pkgconfig file. The corresponding library soname would then be something like libfoo.so.1.

Not sure how far you can get with symbol versioning when targeting Linux, BSDs, OSX, Windows, so classical library soname and header location is probably the way to go.

[ndim]

As we only need a pointer to that type in libavrdude.h, we could use an incomplete type in libavrdude.h, without needing to include avrintel.h from libavrdude.h.

That would be my proposed resolution to this issue.

[drmpjz]

Thanks a lot for your serious consideration of this issue!
As for the "are there users of libavrdude out there part", I know of at least one and that is actually the reason why I am building avrdude :-)
My wife is working in STEM/MINT education and one of the systems they use is the Bob-3 microcontroller (https://www.bob3.org/en/) which uses bobdude (https://www.bob3.org/de/details/software) to transfer the Hex code to the microcontroller. Currently this is sitting on-top of a very old version of avrdude and I am trying to submit a patch to the maintainer of bobdude to bring it up to a modern version. It worked with 7.2 after I submitted a patch earlier. Unfortunately I had an accident and was delayed in sending the patch after 7.2 was released. So I was just about to enter "send" on the patch submission (literally!) when I noticed 7.3 was out and decided to double check for new issues before submitting a problematic patch.

[ndim]

For completeness' sake: My role here is that I have been involved with Fedora's avrdude packages since about 2011, and that I have recently contributed a few simple fixes around building avrdude, mostly with the autotools based buildsystem. So I lack the institutial knowledge a seasoned member of the avrdude development team would know, such as e.g. libavrdude users - and I want to learn more.

[ndim]

As we only need a pointer to that type in libavrdude.h, we could use an incomplete type in libavrdude.h, without needing to include avrintel.h from libavrdude.h.

That would be my proposed resolution to this issue.

Is the avr_locate_config() function even supposed to be part of the public API? The only use of the avr_locate_config() function appears to be in src/avrpart.c, which is part of libavrdude itself, but not the avrdude utility. I do not see any other compilation unit using avr_locate_config()... so this might be just a library internal function?

In that case, we could remove avr_locate_config() from libavrdude.h completely, and therefore the #include "avrintel.h" as well.

[dl8dtl]

PUBLIC_HEADER "libavrdude.h;avrintel.h"

seems to do the trick (still no cmake expert, only stackoverflow knowledge...)

The idea behind libavrdude.h was that it's exactly one public header file that is needed and needs to be published.

There used to be a dozen (or more) files in the past, and we once cooked the public part into a single file.

So the correct method would be ndim's proposed solution to use a pointer to an incomplete struct.

[dl8dtl]

Is the avr_locate_config() function even supposed to be part of the public API?

Good question. ;-)

[Youw]

@mcuee yes, from CMake perspective, updating PUBLIC_HEADER would do the trick.

But @ndim is right. No need to actually include the header. Forward-declare some types for the API function will be enough.

[stefanrueger]

TLDR; I think it's best to install avrintel.h as sibling file to libavrdude.h.

The long of it.

As AVRDUDE obtained a better model of the 350+ parts, 150+ programmers and their interaction, it needed to know more about the parts than was reasonable to put into the user-modifiable avrdude.conf file; amongst the new things were interrupt vector names (needed by urclock), bit-level fuse configuration (needed by the terminal config command) and register file info (needed by terminal regfile). These tables and some access functions ended up in avrintel.[ch]. At some point there was a request to put avrintel into libavrdude, which makes sense, as avrintel.c contains stuff that might be useful for projects that roll their own programmer interface.

While libavrdude.h brought together a dozen or so individual header files that the AVRDUDE project used to have, it looked impractical to put the contents of avrintel.h into it because both avrintel.c and avrintel.h are generated by an ugly meta-program that sources and consolidates intelligence from avr-libc, the .atdf files, avrdude.conf, errata files and some hand-inserted errata from data sheets. When a new .atdf comes out or new errata become known, the meta-program generates a new avrintel.h file.

Maintenance-wise the easiest is to break the rule that there is only one header file for libavrdude and put a second header file as a sibling into the directory wherever libavrdude.h lives. Other solutions are possible, but forward-declaring some types for the API won't give the libavrdude user the same functionality as the full header file.

[ndim]

If it makes sense to ship additional header files, might I suggest to treat those as internal headers and implementation details not to be included directly by library users?

Those additional header files might then clutter PREFIX/include a bit more, but at least they would not clutter library users' source files.

Then libavrdude users would then only ever deal with the one header file, and any other header files would never be part of the public API and could therefore be renamed and moved and removed (provided their former content is provided in another way).

So libavrdude.h would do something like

#define LIBAVRDUDE_INCLUDE_INTERNAL_HEADERS
#include <avrintel.h>
#undef  LIBAVRDUDE_INCLUDE_INTERNAL_HEADERS

while avrintel.h would contain something near the top like

#ifndef LIBAVRDUDE_INCLUDE_INTERNAL_HEADER
#error Do not directly #include <avrintel.h>. Do #include <libavrdude.h> instead.
#endif

Perhaps another name for avrintel.h might be better there, e.g. libavrdude-avrintel.h similar to the wayland-*.h header files. But that filename could always be changed later it the internal header names are not exposed to libavrdude users.

[stefanrueger]

treat [avrintel.h] as internal headers and implementation details not to be included directly by library users

Yes, this should work well and this is how the current use is intended. Admittedly, there is still one instance of #include "avrintel.h" in an avrdude source file but that line can be removed without harm. The suggested changes would prevent users against using avrintel.h as a standalone include, and I support this.

Perhaps another name for avrintel.h might be better there, e.g. libavrdude-avrintel.h

Good idea!

adding #include "avrintel.h" and the avr_locate_config function appears like an unintended change

Both were intended: the former to ensure user code does not need to include avrdude.h (seeing that avrintel.c has become part of libavrdude) and the latter was my gut-feeling that someone "out there" might reasonably want to use that function. Note that it is not solely avr_locate_config() why public access to avrdude.h is needed; other functions such as avr_locate_register_file() also need it. Admittedly, it is long, but it does expose register files and configuration bitfields for fuses on the granularity of individual parts of the some 350 parts. So, if someone wanted to write an uploader/downloader GUI for a m328pb board they could do so without including the tables for all processors.

I do not see any changes to library symbol versioning or soname or similar

Guilty as charged!

libavrdude is kind of the Cinderella of the AVRDUDE project though there has been some push in the past to bring it more to the foreground. Apparently people were building GUI versions of AVRDUDE on top of its binary executable. Jörg @dl8dtl always quite rightly expressed that a more principled way of doing so is to use the functions in libavrdude and, instead of using AVRDUDE's main.c, roll their own main.c. However, there have been quite a few changes to AVRDUDE that, explicitly or implicitly, would change the API/ABI, and therefore should change its version number (but didn't). We have not (yet) created a documentation for libavrdude functions, functionality and philosophy (see #1057, at best users would be able to inspect how functions are used in the AVRDUDE project) and sometimes functions would change what they do (though I have made it part of my review process to ask the question *How does this PR affect libavrdude). There are other issues surrounding libavrdude: #1414 #819 and #774

However, all things considered, I think libavrdude is useful and we should take advice from experts such as @ndim to improve how we build/make/document it within the limited resources that we have.

[mcuee]

libavrdude is kind of the Cinderella of the AVRDUDE project though there has been some push in the past to bring it more to the foreground. Apparently people were building GUI versions of AVRDUDE on top of its binary executable. Jörg @dl8dtl always quite rightly expressed that a more principled way of doing so is to use the functions in libavrdude and, instead of using AVRDUDE's main.c, roll their own main.c. However, there have been quite a few changes to AVRDUDE that, explicitly or implicitly, would change the API/ABI, and therefore should change its version number (but didn't). We have not (yet) created a documentation for libavrdude functions, functionality and philosophy (see #1057, at best users would be able to inspect how functions are used in the AVRDUDE project) and sometimes functions would change what they do (though I have made it part of my review process to ask the question *How does this PR affect libavrdude). There are other issues surrounding libavrdude: #1414 #819 and #774

However, all things considered, I think libavrdude is useful and we should take advice from experts such as @ndim to improve how we build/make/document it within the limited resources that we have.

#819 is more of a generic issue.

• Using libavrdude as a standalone library #819

The following two Issues which may be difficult to fix and require more efforts.

• Remove all calls to exit(3) from libavrdude library functions #774
• Unable to build libavrdude dynamic library under Windows and macOS #1414

And in order to use libavrdude more effectively, #1057 on documentation (as well as examples) is good to be improved.

• To improve the documentation of libavrdude functions #1057

[stefanrueger]

I have created PR #1686 to follow @ndim's recommendation. All that's left doing is to

• Effect the change PUBLIC_HEADER "libavrdude.h;libavrdude-avrintel.h" in src/CMakeLists.txt
• Update the libavrdude version number

  avrdude/src/CMakeLists.txt

  Lines 282 to 283 in 0179026

  	VERSION 1.0.0
  	SOVERSION 1

  to something sensible; I like the idea for this version number to be simply avrdude's, say, 7.3.20240214

@ndim What do you think? If I merge PR #1686 as is, could you make the above changes in your PR #1681?

[ndim]

@stefanrueger I have started to look into the library versioning with cmake only a few hours ago. I have not figured out yet how they work.

The libtool versioning scheme looks weird and complex at first sight, but it does allow adding parts to a dynamic library's ABI without needing to relink software which is dynamically linked to that library.

The cmake library VERSION and SOVERSION are different, and I have not yet figured out how to use them.

BTW I started looking into the library version while trying to script the Autotools build system to get that information from CMakeLists.txt.

[ndim]

I have created PR #1686 to follow @ndim's recommendation. All that's left doing is to

* Effect the change `PUBLIC_HEADER "libavrdude.h;libavrdude-avrintel.h"` in `src/CMakeLists.txt`

* Update the libavrdude version number https://github.com/avrdudes/avrdude/blob/0179026e09d19833e7dea06ed8db19f971138e4b/src/CMakeLists.txt#L282-L283
   to something sensible; I like the idea for this version number to be simply avrdude's, say, 7.3.20240214

@ndim What do you think? If I merge PR #1686 as is, could you make the above changes in your PR #1681?

As many things have been added to libavrdude.h after the last time the libavrdude VERSION and SOVERSION were changed in src/CMakeLists.txt to 1.0.0 and 1 (and libtool -version-info, for that matter), I would say that the addition of these three functions will not make a significant difference for library users with VERSION and SOVERSION remaining as they are.

I have verified that both the cmake and the autotools build both produce a libavrdude.so.1.0.0 with an soname of libavrdude.so.1, so we have consistent names here. That is very good.

Making library ABI version numbers the same as CMAKE_PROJECT_VERSION is a bad idea. ABI changes and release version numbers are very different things.

My suggestion for the version numbers is to keep VERSION, SOVERSION, and libavrdude_la_LDFLAGS as it is for now, and then later research, implement and document a scheme for VERSION and SOVERSION and translate that to _LDFLAGS. Possibly independently from PR #1681.

The installation of libavrdude-avrintel.h can happen now, as far as I am concerned.

[ndim]

BTW... is the union in

avrdude/src/libavrdude.h

Lines 1386 to 1397 in a826792

	typedef struct {
	int size, sigsz, type;
	char *errstr, *warnstr, *str_ptr;
	AVRMEM *mem;
	union {
	float f;
	double d;
	int64_t ll;
	uint64_t ull;
	uint8_t a[8];
	};
	} Str2data;

unnamed on purpose?

That unnamed union imposes on any compilation unit which includes libavrdude.h that it must be compiled as C11 or later, as unnamed unions were introduced by C11.

Or is C11 officially the C language standard which libavrdude.h requires?

[stefanrueger]

unnamed union

Good catch. This is for another time. I might review the project in the future to see whether we can restrict ourselves to C99

[stefanrueger]

The installation of libavrdude-avrintel.h can happen now, as far as I am concerned.

I just merged PR #1686 so this should be in git main

[stefanrueger]

My suggestion for the version numbers is to keep VERSION, SOVERSION, and libavrdude_la_LDFLAGS as it is for now

OK, that is a good suggestion

[mcuee]

Or is C11 officially the C language standard which libavrdude.h requires?

It may be difficult as MSVC is still evolving the support of C11.
https://developercommunity.visualstudio.com/t/Add-C11-Support/387315?beforeInstall=true (Done)
https://developercommunity.visualstudio.com/t/Finalize-C11-support-for-threading/1629487?q=Node+JS+support+ (under review)
https://developercommunity.visualstudio.com/t/Missing-threadsh-in-MSVC-178/10514752 (fixed in Jan 2024)

[mcuee]

Good catch. This is for another time. I might review the project in the future to see whether we can restrict ourselves to C99.

@stefanrueger

Please take a look at the MSVC C99/C11 comformance limitations to see if we can move to C11. Our CI is using the pretty new MSVC version from VS2022 so basic C11 features should work with some exceptions).
https://learn.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=msvc-170#c-standard-library-features-1

[mcuee]

I think we already use C11 in CMake.

avrdude/CMakeLists.txt

Lines 23 to 27 in 5c1649d

	cmake_minimum_required(VERSION 3.14)
	project(avrdude VERSION 7.3 LANGUAGES C)
	set(CMAKE_C_STANDARD 11)
	set(CMAKE_C_STANDARD_REQUIRED True)

BTW, we use C++ for MSVC here.

avrdude/src/CMakeLists.txt

Lines 63 to 64 in 5c1649d

	if(MSVC)
	enable_language(CXX)

[ndim]

The language standard used to build libavrdude the library and avrdude the utility can be different from the language standard other users of libavrdude can use.

[mcuee]

The language standard used to build libavrdude the library and avrdude the utility can be different from the language standard other users of libavrdude can use.

Good point. Thanks.

[mcuee]

I think PR #1688 is good to go and it will fix this issue.

• libavrdude.h fixes and installation of libavrdude-avrintel.h #1688

[stefanrueger]

Solved by PR #1686 and #1688