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

added cmake build #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

added cmake build #109

wants to merge 1 commit into from

Conversation

linux-fan-dave
Copy link

No description provided.

@peterhoeg
Copy link
Member

Hi @linux-fan-dave, do you mind sharing some details regarding the purpose of this PR?

@probonopd
Copy link

Looks like @linux-fan-dave prefers CMake over autotools, and I share this preference.

@linux-fan-dave
Copy link
Author

Ups sorry. I thought I did provide a description. I simply added a CMake build file as a alternative to the autotools build. If you already use CMake it is really easy to add dependencies to other CMake projects. Autotools do not integrate quite as nice.

@fsateler
Copy link
Contributor

fsateler commented Dec 5, 2016

This cmake script is not equivalent to the autotools one. Differences I can find:

  1. Does not autodetect page size.
  2. Does not include tests.
  3. Enables C++11 mode
  4. Does not install manpage

@linux-fan-dave
Copy link
Author

Would the CMake build file be accepted if it were without the mentioned problems?
To 1:
What does autoedect page size mean? I saw a default in the configure and a paramter that can overwrite the auto. This is the behaviour in cmake as well. It has a default / auto and a paramter that can be used to overwrite.
To 2:
Yes this is true. Could be done.
To 3:
I was not able to compile without C++11 enabled - but I could investigate further.
To 4:
I could add that.

@fsateler
Copy link
Contributor

fsateler commented Dec 6, 2016

@linux-fan-dave I am not a maintainer so I can't answer your overall question (but in general I like cmake more than autotools so +1 from me). Of course you should wait to see if the maintainer wants this change before doing more work on this.

As to 1, there is this line in configure that queries getconf to ask for the page size: https://github.com/NixOS/patchelf/blob/4034c6d/configure.ac#L17

Moreover, your parameter is not overridable in the command line. It should probably use the CACHE option, and provide a short help string.

@vcunat
Copy link
Member

vcunat commented Dec 7, 2016

If you already use CMake it is really easy to add dependencies to other CMake projects. Autotools do not integrate quite as nice.

I don't get what dependencies you mean. Is there some other task than locating the patchelf executable? Surely cmake can handle that easily already. EDIT: please, no offense meant; it's more about my curiosity ;-)

@fff7d1bc
Copy link

fff7d1bc commented Dec 7, 2016

In general, cmake allows you to easly "add_subproject()" which is useful when you want to link (for example statically) against another library, then cmake will build another cmake project as dependency and provide you the thing.

Here however, patchelf is a application not library, so no much useful.

This said, CMake sucks much less than Autotools and I am honestly surprised when i see a software that is not from '90 but decide to start with autotools. If I could I would replace autotools everywhere with CMake. :)

@vcunat
Copy link
Member

vcunat commented Dec 7, 2016

I'm getting off-topic: my experience is that autotools-based tend to be more robust – when you have some "unusual" demands, and I was surprised by that. In particular I dealt with wanting to put different parts of the application to different prefixes (--libdir, --mandir) which worked mostly flawlessly on more than a hundred nixpkgs packages, and I did some cross-compilation.

EDIT: I don't mean to deny that much of autotools feels terribly hacky and it's showing its age, but I don't think the situation is as clear as "cmake is always better for new projects". (Besides, patchelf is over ten years old now.)

@linux-fan-dave
Copy link
Author

In our case we have a cmake based project which needs to patch some binaries before beeing deployed. We therefore included patchelf as a third party project to our project and compile it. It therefore can be used in a postbuild step.
A cmake based project can easily be added to our project, whilst using the autotools from our cmake build is a bit of a hassle. If there is any interest in adding it to patchelf - I would fix the problems with cmake to be added upstream to patchelf.

@ghost
Copy link

ghost commented Dec 8, 2016

to detect automatically Linux PAGE_SIZE you could write cmake execute_process!

Something like this will do the trick

execute_process(COMMAND ${CMAKE_COMMAND}  getconf PAGESIZE OUTPUT_VARIABLE PAGE_SIZE_VAR) 

add_definition (-DPAGE_SIZE=${PAGE_SIZE_VAR})

But untested though...

Copy link
Contributor

@DerDakon DerDakon left a comment

Choose a reason for hiding this comment

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

This misses install() steps to actually place the compiled executable somewhere.

@@ -0,0 +1,28 @@
project(patchelf)
cmake_minimum_required(VERSION 2.8)
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply require version 3.1 here and enable the C++11 mode with it unconditionally below.

@lisanna-dettwyler
Copy link

I'm going to see if I can get this working... fingers crossed

@concatime
Copy link

concatime commented Jan 31, 2022

I added Meson on my fork. I can create a PR if you are interested. It compiles successfully with muon + samurai (neither Python nor a C++ compiler are required, only a C99 compiler). That being said, tests are not yet implemented.

@wdlkmpx
Copy link

wdlkmpx commented Jun 14, 2023

Both cmake and meson projects should be added, the community must address the bugs until the projects are 100% compatible with the autotools stuff.

The maintainer only has to mention in the readme file that only the autotools project is officially supported, any bug in the other projects is ignored unless the community provides patches, sounds reasonable doesn't it?

And perhaps to avoid any confusion, the projects should go inside a subdir, i.e. contrib, and should be able to run from that location. This is what lz4 does

When it comes to cross-compiling, autotools offers the shortest solution (--build=xxx --host=yyy), but other build systems offer better syntax (specially meson), several times faster configure, out-of-tree builds, support for Windows and non-posix systems, etc...

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.

10 participants