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

Some fixes to cmake files #79

Merged
merged 7 commits into from
Jun 4, 2022
Merged

Some fixes to cmake files #79

merged 7 commits into from
Jun 4, 2022

Conversation

htot
Copy link
Contributor

@htot htot commented May 14, 2022

Hi,

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

  • when enabling OpenMP also link against openmp lib, otherwise still not enabled
  • not only build test tools but also install them (this is needed for Yocto recipe to pick them up and package)
  • also build base64 command line encoder
  • shutup warnings on recent versions of cmake (optional, @burn care to review this?)
  • updated edison x86_64 benchmarks (optional)

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?

test/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@BurningEnlightenment
Copy link
Contributor

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
Copy link
Contributor Author

htot commented May 16, 2022

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
Copy link
Contributor

BurningEnlightenment commented May 19, 2022

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

CMakeLists.txt Outdated Show resolved Hide resolved
bin/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@dotnwat
Copy link

dotnwat commented May 20, 2022

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.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@htot
Copy link
Contributor Author

htot commented May 26, 2022

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

Copy link
Contributor

@BurningEnlightenment BurningEnlightenment left a comment

Choose a reason for hiding this comment

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

@aklomp I think this is ready to be merged :shipit:

(I think a sane quality gate going forward would be two approvals from contributors who made substantial cmake contributions)

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Owner

@aklomp aklomp left a comment

Choose a reason for hiding this comment

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

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.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@BurningEnlightenment
Copy link
Contributor

BurningEnlightenment commented Jun 2, 2022

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.

b3c2979 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
Copy link
Contributor Author

htot commented Jun 2, 2022

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
Copy link
Owner

aklomp commented Jun 2, 2022

@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
Copy link
Owner

aklomp commented Jun 2, 2022

@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
Copy link
Contributor Author

htot commented Jun 2, 2022

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.

@aklomp aklomp mentioned this pull request Jun 3, 2022
htot added 5 commits June 4, 2022 21:13
Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
CMake `3.9` introduced the library target `OpenMP::OpenMP_C` which is
way more portable than the old compiler flag based approach. Due to the
required version bump we surveyed the available CMake versions on widely
used OS distributions. Considering 3.10.2 is oldest suported in
Ubuntu 18.04 LTS (EOL april 2023): https://packages.ubuntu.com/search?keywords=cmake
we choose `3.10.2` as the new required version which only excludes RHEL7.

https://lists.llvm.org/pipermail/llvm-dev/2020-April/140578.html
Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
8d0674e contains install commands for all test binaries. This is convenient for tools
like Yocto to pick them up and package them.
However, in the general case library consumers don't need the test binaries at all
do not need to be compiled or installed.

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
Signed-off-by: Henrik Gaßmann <BurningEnlightenment@users.noreply.github.com>
Co-authored-by: Henrik Gaßmann <BurningEnlightenment@users.noreply.github.com>
Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
@htot
Copy link
Contributor Author

htot commented Jun 4, 2022

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
Copy link
Owner

aklomp commented Jun 4, 2022

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

  • The changes to the benchmarks do not fit with the theme of this issue and are sort of sneaked in.
  • This patchset first introduces some code (the workaround to install an executable with the same name as a library) and then removes it in a subsequent commit. I'm a fan of making commits atomic and composable: they only add or remove something, and do so in isolation without depending on other commits in the patch set. Anwyay, this project has no submission guidelines yet, so it's not a big deal.

Thanks everyone for contributing!

@aklomp aklomp closed this in 43e3707 Jun 4, 2022
@aklomp aklomp merged commit 43e3707 into aklomp:master Jun 4, 2022
aklomp pushed a commit that referenced this pull request Jun 5, 2022
The recent changes in #79 accidentally disabled the compilation of the
CTest tests. Enable the tests again, and add a check for missing tests
to avoid the same situation in the future.

Closes #84.
@aklomp aklomp mentioned this pull request Jun 6, 2022
7 tasks
@aklomp aklomp added this to the v0.5.0 milestone Jun 6, 2022
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.

5 participants