Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autotools #8

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Autotools #8

wants to merge 18 commits into from

Conversation

barak
Copy link

@barak barak commented Apr 25, 2014

Complete rewrite of the build system using autotools.

@rofl0r
Copy link
Contributor

rofl0r commented Dec 25, 2016

even though i'm not a big fan of autotools (but its still better than alternatives: cmake, scons etc) this would probably at least fix the current issues in the build system:

  • the osflags script is completely unusable for crosscompiling or non-standard filesystem layouts since it has hardcoded a number of header locations. so if your host system has systemd installed, you will compile systemd support into it even though your target toolchain doesn't have the corresponding headers or libs.
    but if you need systemd support, but have your headers in a different prefix, they won't be picked up even though the toolchain itself is aware of the prefix. the usage of hardcoded header file paths is completely flawed. all such tests should be done by referencing a header from a c file like #include <headeriwannatest.h> and some typedef and try to compile it with the user-supplied $CC.
  • the usage of uname is lame and prevents crosscompiling as well. if you want to crosscompile for mac from linux, uname will detect the host system as linux and will try to compile the linux version.
  • due to the inability (or actually complexity) of make to run configure-style compile tests, the author uses a broken forest of #ifdef with a list of systems that provide daemon(), instead of testing for whether the current toolchain provides it or not. thus the build fails on musl libc and any other system that has daemon, but is not yet part of the ifdef chunk.

each of those points is actually a bug that should be adressed and filed as an issue, but i can't be bothered to register at yet another trac.

@rofl0r
Copy link
Contributor

rofl0r commented Dec 25, 2016

doh. i forgot the 4th point that is:

@barak
Copy link
Author

barak commented Dec 25, 2016

(Just to be clear: I also hate autotools. To really know autotools is to really hate autotools. But it has been forged in the crucible of time and passed through to the other side.)

Do you want me to try to forward-port this patch to your repo tip?

@rofl0r
Copy link
Contributor

rofl0r commented Dec 25, 2016

@barak: i (ab)used this PR here to raise my concerns about the existing build system, since the github issue tracker here is locked down (probably the author invested so much time into getting his trac running that he feels somehow attached to it, even though the github issue tracker is much better integrated, and would free him of the maintenance work).
since there was apparently no interest in your work i would probably refrain from doing more unless the repo owner (@yarrick) shows up and comments his wishes.

only non-trivial conflict is the CC option -std=c99, which I defer
src/iodined.c: In function ‘read_dns’:
src/iodined.c:2062:34: error: invalid application of ‘sizeof’ to incomplete type ‘struct in6_pktinfo’
  char control[CMSG_SPACE(sizeof (struct in6_pktinfo))];
                                  ^
@barak
Copy link
Author

barak commented Dec 28, 2016

I merged these changes to the development tip, and made a few small changes to integrate them with the intervening changes to the code and to the rest of the system.

@yarrick
Copy link
Owner

yarrick commented Dec 28, 2016

Maybe it is time to switch.. Building for windows or android is really hacky now. I also think the android version doesnt run on modern android already because of some link flags - maybe it will be easier to fix with some autotools magic.

It seems the thing in the patch setting the -DDARWIN for mac includes a version number and the compiler does not like that.

Regarding trac, I am fine with using the bug tracker here, I am just too lazy/busy to move the existing issues and wiki pages.

@barak
Copy link
Author

barak commented Dec 28, 2016

SUCCESS!!!


# Checks for headers

AC_CHECK_HEADERS([arpa/inet.h fcntl.h netdb.h netinet/in.h stdint.h stdlib.h string.h sys/ioctl.h sys/param.h sys/socket.h sys/time.h syslog.h termios.h unistd.h systemd/sd-daemon.h selinux/selinux.h])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stdint.h stdlib.h string.h unistd.h are standard C headers (even since C89).
all the other headers apart from sys/ioctl.h, sys/param.h and the systemd and selinux headers are standard POSIX headers. so i see little point in testing for their existence.


AC_FUNC_FORK
AC_FUNC_MALLOC
AC_CHECK_FUNCS([alarm dup2 inet_ntoa memset select socket strcasecmp strchr strdup strerror strrchr strstr])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh... if the C library doesn't even supply malloc and memset then you cannot compile anything.
out of the functions listed here all are standard C or POSIX funcs, apart from strdup, dup2 and inet_ntoa.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually i just have a PR pending doing just that, removing nonsensical checks for functions that are in any C library that came out in the last 15 years: tinyproxy/tinyproxy#66

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, you should check for daemon() and replace the ifdef-hell in src/common.c with a single #ifndef HAVE_DAEMON. apart from that this repo has a history of not checking for anything, so it's definitely not necessary to now introduce a ton of checks for standard functionality that just make the build much slower.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Go for it! Most of the checks are not really sensible, and the #ifdefs in the code proper could be normalized to use the HAVE_xxx properties generated by autoconf almost exclusively.

(I used autoscan and plugged along from there pretty brainlessly.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants