aklomp / base64

Fast Base64 stream encoder/decoder in C99, with SIMD acceleration
BSD 2-Clause "Simplified" License
866 stars 162 forks source link

Some fixes to cmake files #79

Closed htot closed 2 years ago

htot commented 2 years ago

Hi,

Here are some fixes after the recent pull of cmake support.

My motherboard i7-4770 @ 3.4 GHz DDR1600 died, I won't be able to update these. I could provide a set on i7-10700 CPU @ 2.90GHz DDR4 3200?

BurningEnlightenment commented 2 years ago

shutup warnings on recent versions of cmake

These stem from CMake behaviour changes. The given changes in CMP0060, CMP0065, CMP0082, CMP0127 shouldn't affect us in a bad way. Whereas CMP0060 is probably caused by the OpenMP target provided by newer Versions of cmake

htot commented 2 years ago

shutup warnings on recent versions of cmake

These stem from CMake behaviour changes. The given changes in CMP0060, CMP0065, CMP0082, CMP0127 shouldn't affect us in a bad way. Whereas CMP0060 is probably caused by the OpenMP target provided by newer Versions of cmake

The warnings are a bit noisy when they are enabled. I have no idea if it is normal to individually turn them off like this. We can drop this on if you think it is better?

BurningEnlightenment commented 2 years ago

Sorry, for stringing you by, but I'm currently occupied with an university project. I think I might be able to further review this at the weekend. W.r.t. requiring a newer cmake version I did some research a few months a go:

I think it should be considered to require an even newer version as a similar discussion on llvm-dev last year [1] revealed that all current LTS platforms support at least 3.6.1 (or 3.13.4 if one is willing to sacrifice RHEL 6 and Debian 9 support).

So from my POV 3.13.4 would be fine. However, I really would like to hear from other cmake users first 😅 like @madebr, @dotnwat or @vglavnyy

dotnwat commented 2 years ago

So from my POV 3.13.4 would be fine. However, I really would like to hear from other cmake users first 😅 like @madebr, @dotnwat or @vglavnyy

We should be fine with 3.13+, thanks for asking. But FWIW, Ubuntu Bionic LTS still has standard support for another year and that has 3.10.

htot commented 2 years ago

@BurningEnlightenment I've taken your updates, squashed and force pushed. It works well for me. Thanks for reviewing.

BurningEnlightenment commented 2 years ago

This commit really needs some more explanation in the commit body about why the tests are being disabled. Without context, I have no clue why this change was made, and my inclination is to reject it.

b3c29792f6cc58f2d006f3b5ada5a41626eb31bc contains install commands for all test binaries, because yocto (the package manager used by htot) won't pick them up otherwise which in turn is desirable if you want to verify that your cross compiled binaries work. However, in the general case library consumers don't need the test binaries at all i.e. they are bloat and not worth to be compiled or installed. So I thought we should make the majority use case the default and put the configuration burden on the developing minority. Alternatively we can add another config option which controls whether the test binaries will be installed or not and turn that off by default instead.

htot commented 2 years ago

Edit: in reference to 8abb204a

This commit really needs some more explanation in the commit body about why the tests are being disabled. Without context, I have no clue why this change was made, and my inclination is to reject it.

I believe as a default, the idea is only the library is built. But with the cmake gui you can change these defaults during configure (as well as turn on openmp, turn off sse4 etc).

For me, the Yocto recipe overrides all defaults and builds everything, so the defaults don't matter. And then everything is packaged into it's respective deb package (-dev, -dbg, -tools etc.).

To clarify: Yocto is a project that builds tools to build distributions. Similar that Debian has a large set of tools to build/test/package/deploy to repositories. Except Yocto builds your distribution. The base tool is bitbake which uses recipes to retrieve sources (from github f.i.), build, install on image, package and place in a repository. It can build distributions to use dnf, apt or opkg as package manager.

So, let's say you have some device with embedded linux, the image could be built with bitbake using 1000's recipes fully automated (and reproducible too because the first thing it is does it to start building it's compile chain), building U-Boot, linux, systemd etc. And one recipe will build libbase64.

aklomp commented 2 years ago

@BurningEnlightenment When you put it like that, it makes total sense. If the user is running CMake, the assumption is that they're building libbase64 as a part of a larger whole, and are only interested in the library itself.

The commit would be much better if it included that reasoning, because the commit is the output of this process, and also the artifact that reflects the change. It should strive to be self-describing.

aklomp commented 2 years ago

@htot Thanks for clarifying your use-case a bit. I'm slightly familiar with Yocto and have run into it in the past when trying to recompile some software for an embedded board. Man, what a monster, I much prefer Buildroot :)

htot commented 2 years ago

I'll fix up the commits based on above discussion.

@aklomp Yes, I heard buildroot is more lightweight. And I don't like python. But it really is a brilliant piece of work. It sometimes makes me wonder why Suse/Redhat/Debian don't ditch their own tooling and collaborate on this :-)

I case you wonder, my recipe is here. As you can see, it pulls from aklomp/master, applies my patches (for now). As it uses cmake there is not much libbase64 specific to do, except turning on the correct build options and splitting out tools to a separate packge.

htot commented 2 years ago

I think I resolved all comments now and set Kate to always removing training white spaces. I squashed the 2 commits and added suggestions to commit messages. Thanks all for review.

aklomp commented 2 years ago

Thanks @htot. I'll merge this patchset, but I do think it's a bit messy:

Thanks everyone for contributing!