vdudouyt / stm8flash

program your stm8 devices with SWIM/stlinkv(1,2)
GNU General Public License v2.0
403 stars 183 forks source link

Creates a standard autotools setup. #89

Open ceneblock opened 6 years ago

ceneblock commented 6 years ago

I wasn't a fan of the static Makefile, this will turn the project into using autotools which detects missing libs and sets everything "auto-magically" for the end user.

It also makes life easier for the maintainers.

spth commented 6 years ago

While I can see the advantage of using autotools to detect libusb, using autotools also adds some overhead. It makes it harder for users to compile the program (especially since configure is not in the repository).

Also, why do you use all those

AC_CHECK_HEADER_STDBOOL
AC_TYPE_SIZE_T
AC_TYPE_SSIZE_T
AC_TYPE_UINT16_T
AC_TYPE_UINT32_T
AC_TYPE_UINT8_T

instead of just requiring a C99-compliant C compiler?

Also in these checks:

AC_FUNC_MALLOC
AC_CHECK_FUNCS([memset strcasecmp strdup strerror strrchr])

The only ones that make sense to me are strcasecmp and strdup, as the other ones are standard C functions available at least on any hosted C implementation.

And for

AC_CHECK_HEADERS([fcntl.h malloc.h stddef.h stdint.h stdlib.h string.h strings.h sys/param.h termios.h unistd.h])

malloc.h is no longer used in stm8flash, and again many of the headers are standard C headers.

Philipp

ceneblock commented 6 years ago

My apologies for not including the configure file. I can get that in there no problem

In regards to the info in configure.ac those were all pulled in from autoscan. If I understand correctly, the line "AC_PROG_CC_STDC" in configure.ac should ensure that C99 is present (at this current time); however, I can add additional checks to verify that C99 is indeed present.

I can see an argument both ways for just including a Makefile and having the configure; however, the configure ensures that all libraries are present and accessible while providing additional output if they are not. This also follows the "standard" for GNU packages, in addition to the INSTALL file explaining how to properly compile the program.

Thank you for taking the time to respond to me!

spth commented 6 years ago

AC_PROG_CC_STDC will AFAIk still fall back to a C80/C90 compiler. I am not an autotools expert, but I think AC_PROG_CC_C99 and checking ac_cv_prog_cc_c99 is still needed.

Do we really want a separate include directory? Since we are just building an executable, and no libraries, IMO, the he4ader files could just go into src.

Philipp

ceneblock commented 6 years ago

From https://www.gnu.org/software/autoconf/manual/autoconf-2.60/html_node/C-Compiler.html:

If the C compiler cannot compile ISO Standard C (currently C99), try to add an option to output variable CC to make it work. If the compiler does not support C99, fall back to supporting ANSI C89 (ISO C90).

After calling this macro you can check whether the C compiler has been set to accept Standard C; if not, the shell variable ac_cv_prog_cc_stdc is set to ‘no’.

So, you're correct. I'll make the changes.

Looking at https://ftp.gnu.org/gnu/hello/hello-2.10.tar.gz, they have header files, but do not have a separate include/. That said, I think it looks cleaner, but it is up to the project on that one. I'll do whatever is needed.

spth commented 3 years ago

Any news on this one?

kasunch commented 3 years ago

Just out of curiosity, why don’t you use a modern build system generator like CMake?

ceneblock commented 3 years ago

Just out of curiosity, why don’t you use a modern build system generator like CMake?

Because I don't know CMake. :)

Everything I do is on Linux or FreeBSD so the GNU build system works perfectly for me.

CMake is probably a better choice. If you'd like to do that then I'll close my PR. Looks like I need to fix this anyways, although a 4 year old PR pretty much tells me that this won't get fixed.

spth commented 3 years ago

Well, I don't know cmake either. My experience with autotools is limited, but I'd still feel more comfortable reviewing a autotools pull request than a cmake one.

My comment above is partially obsolete now, as AFAIK AC_PROG_CC_C99 is now deprecated.

I don't have experience with building stm8flash on systems other than Unices, and wonder how the autotools setup will affect building for Windows.

kasunch commented 3 years ago

The Makefile used at the moment is uncomplicated. So writing a CMakeLists.txt file would be straightforward. I have a fair share of experience with Cmake, so I can contribute to this.

After looking at the Makefile, the question I have is if moving to Autotools or CMake or anything else is worthy at the moment. As I see, the only dependency is libusb. Use of the of pkg-config in the Makefile is probably fine and simple for the moment.

If moving to a build system generator is worthy, I suggested CMake since it is modern and has a wider support for dependency resolving. However, I don’t have a strong opinion on this over Autotools.