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

build: migrate build system to autotools #54

Closed
wants to merge 1 commit into from
Closed

build: migrate build system to autotools #54

wants to merge 1 commit into from

Conversation

bostjan
Copy link
Contributor

@bostjan bostjan commented Jun 5, 2015

Hi,

I've created proper autotools build environment to support the most common build procedure:
./configure
make
make check
make install

Motivation:
iniparser is a neat library that I integrated into Snoopy Logger. But I had to hackishly add it (copy contents, create custom make rules) which was not elegant. Switching iniparser to autotools-based build system allows anyone to add it to their project with as little hassle as possible, using nested subpackages feature.

The only trick is to disable shared builds of iniparser, so parent project links itself with static iniparser library instead of dynamic one. For this, the appropriate switch was created (--enable-subpackage, more info below).

You can see how iniparser is integrated into project that requires it, by means of autotools nested subpackage, here:
a2o/snoopy@82a0c81

Please ask if you have further questions about this.

If possible, please share your opinion about this pull request.

Here follows the commit message:

Iniparser is mostly used as a subcomponent of other projects. Some
of them required it as shared library that is already present on
the target system, while others might want to incorporate it into
own package distribution and build/ldadd it to their final binary/
library statically.

Autotools suite supports a really nice feature called subpackages.
It allows project to have subpackage as it's dependency, and does
recursive ./configure / make handling by itself.

With proper autotools support, iniparser can be just dropped into
another project, add a few settings to configure.ac, lib/Makefile.am
and src/Makefile.am and we are ready to go.

For this to work (to be embedable statically into another project),
another way of building iniparser had to be implemented. It is
reachable by the switch --enable-subpackage. This causes main
library to be built as noinst (not-installing library), which in
turn causes libiniparser to produce only it's static version. This
is the desired result, as this causes parent project to link
iniparser statically into it's libs/binaries.

Additionally, an optional --disable-documentation switch was created, as some systems might not have doxygen program installed on them.


b.

Iniparser is mostly used as a subcomponent of other projects. Some
of them required it as shared library that is already present on
the target system, while others might want to incorporate it into
own package distribution and build/ldadd it to their final binary/
library statically.

Autotools suite supports a really nice feature called subpackages.
It allows project to have subpackage as it's dependency, and does
recursive ./configure / make handling by itself.

With proper autotools support, iniparser can be just dropped into
another project, add a few settings to configure.ac, lib/Makefile.am
and src/Makefile.am and we are ready to go.

For this to work (to be embedable statically into another project),
another way of building iniparser had to be implemented. It is
reachable by the switch --enable-subpackage. This causes main
library to be built as noinst (not-installing library), which in
turn causes libiniparser to produce only it's static version. This
is the desired result, as this causes parent project to link
iniparser statically into it's libs/binaries.
@ndevilla
Copy link
Owner

ndevilla commented Jun 5, 2015

Oh Lord, no! People who really want to have autotools build their project will find a way to include the two sources files that are libiniparser. Same comment for fancy Makefiles: guys these are just two source files! Include them into your build system but do not try to propagate your changes to the trunk here. Anything you submit in terms of Makefile or build facility is likely to be tied into your own system.

@ndevilla ndevilla closed this Jun 5, 2015
@bostjan
Copy link
Contributor Author

bostjan commented Jun 5, 2015

Tnx for comment, I understand :)

@ndevilla
Copy link
Owner

ndevilla commented Jun 5, 2015

Thanks for your understanding, and thanks for taking the time to contribute. Always appreciated!
Elaborating a bit more on this: I have received countless contributions from distrib people to modify the Makefile into what they think is the "standard", which I had to reject. The default, standard Makefile for Debian bears absolutely no relationship with the one from SuSE or RedHat and there is no possible way to merge them all.
Again: a build system is something so specific to each environment that it is completely pointless to try and push anything that claims to be standard. The provided Makefile in this project is purely here to have something to play with quickly.

And yeah I might have a thing or two against autotools, I confess :-)
But that should not discourage anybody from using them.

@bostjan
Copy link
Contributor Author

bostjan commented Jun 5, 2015

Distro maintainers:
Don't they use separate repo for build tools/scripts/files which is mainained either separately, or as an orphaned branch in the clone of upstream repo, with all distro-specific tools in this special branch? I thought so, but my sample pool is tiny.

Autotools:
Not a heavy user here, but once I got familiar with multiple layers of syntax in .am and .ac files, I find it quite pleasant to work with. Even Makefile.ams are more readable than pure Makefiles or .ins (but that is my judgement, again based on tiny amount of samples).

Care to elaborate, what is your grudge against autotools?

PS: Just a quick side q (I know this is OT): does 4.0 have all thread-safety-specific things merged in, right?

b.

@ndevilla
Copy link
Owner

ndevilla commented Jun 5, 2015

You should see my mailbox a few years back... All the major distro guys got in touch to try and get their Makefile in. I might have actually said 'yes' to the first one and reverted after another distro came in with a patch to "make the Makefile more standard". Good thing we have so many standards to choose from!

Autotools: I could write a full book about this. Let me try to be concise:
Why should I replace Makefile's syntax (which I know) by m4 and other autotools-specific syntax (which I do not)?
'make' is just one program. Autotools have a shitload of dependencies, including m4 and perl. Dependencies are only for the person generating the 'configure' script, granted, but as it happens to be me, I do not want to drown under that shitload.
Autotools claim to offer portability to a whole lot of systems I do not care about, and I have no way to test if it would work anyway. Why should I care about archaic exotic platforms and compilers? This is solving a problem I do not have.
Portability among Unixes can be achieved with Makefiles in several reasonably elegant ways. I used to compile much larger projects than iniparser on a dozen Unix flavours with the same Makefile without need for extra tools.
Do you really want to add a few megabytes of shell scripts and other config files to a two-C-file project? Seems a bit overkill.

Emmanuel told me v4.0 includes thread safety but please check.

Nicolas

@bostjan
Copy link
Contributor Author

bostjan commented Jun 5, 2015

Oh, another thing: create CONTRIBUTING.md file or corresponding section in README.md and put this "Oh Lord, no" for build changes in there :)

b.

@bostjan
Copy link
Contributor Author

bostjan commented Jun 5, 2015

:)))))
Point taken :)

@touilleMan
Copy link
Collaborator

PS: Just a quick side q (I know this is OT): does 4.0 have all thread-safety-specific things merged in, right?

Yes, iniparser 4 is designed to be thread-safe, provided you surround it with your own mutex logic. The choice not to thread safety inside the library has been done to provide more freedom for the developer, especially when dealing with it own custom reading logic (i.g. acquiring the mutex, reading plenty of entries in iniparser, then releasing the mutex).
And if you found any bug here, your contributions will be much appreciated ;-)

@bostjan
Copy link
Contributor Author

bostjan commented Jun 6, 2015

Care to elaborate on this part a little bit?

Does only iniparser_load() need to be wrapped in mutex, or whole ini parsing part of the program that uses iniparser?

I glanced over the code and I did not find anything suspicious, except line_status. I did not yet check whether all glibc functions used are the thread-safe ones.

b.

@touilleMan
Copy link
Collaborator

Does only iniparser_load() need to be wrapped in mutex, or whole ini parsing part of the program that uses iniparser?

I would say it really depend of you workflow.

All functions in iniparser have been corrected since 4.0 to be re-entrant. Thus, iniparser_load can be used simultaneously from multiple thread without mutex.

In general the only element should to worry about is the document * parameter that take most of the iniparser functions.

For exemple, if you only read the dictionary (no alteration after loading) you should not need any mutex (given all the read functions take a const dictionary * as parameter, they won't try to modify it)

On the other hand, if you use at some point iniparser_set (which take a non constdictionary *), you need to wrap all the functions (including reading !) to ensure the dictionary is in a coherent state before using it.

@bostjan
Copy link
Contributor Author

bostjan commented Jun 7, 2015

Thanks for excellent explanation. You should put it somewhere more visible (doc/THREADS.*), as this issue's subject is not the best title for this content :)

b.

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.

None yet

3 participants