Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#52699 closed enhancement (fixed)

cmake portgroup version 1.1

Reported by: mkae (Marko Käning) Owned by: mkae (Marko Käning)
Priority: Normal Milestone:
Component: ports Version:
Keywords: Cc: RJVB (René Bertin), mojca (Mojca Miklavec), michaelld (Michael Dickens), ryandesign (Ryan Carsten Schmidt)
Port: cmake

Description (last modified by mkae (Marko Käning))

... as well as Ryan, of course.

Attachments (2)

cmake-Portgroup.diff (8.4 KB) - added by mkae (Marko Käning) 7 years ago.
cmake-PortGroup.diff (9.6 KB) - added by RJVB (René Bertin) 7 years ago.

Download all attachments as: .zip

Change History (31)

Changed 7 years ago by mkae (Marko Käning)

Attachment: cmake-Portgroup.diff added

comment:1 Changed 7 years ago by RJVB (René Bertin)

Most of these changes have already been presented elsewhere (and for a large part, ratified as useful, just never incorporated). I can't find the tickets with a few quick searches, so here's a bit of background.

First off, I've decided to give my variant a higher version, so both Michael's original and my more elaborate portgroup can co-exist (for the time being).

changes include:

  • use CMAKE_BUILD_TYPE=MacPorts. Inspired by Debian's own build type, this allows us to specify all compiler settings via the well-known configure.* commands and exported via the environment. If one of CMake's predefined types is used the corresponding standard options will be *appended* to our options, which will override notably the optimisation options. Some parsing of configure.cppflags, configure.cflags and configure.cxxflags is done to ensure this works as expected, in lines 145-200 .
  • add CMAKE_EXPORT_COMPILE_COMMANDS so that each compiler command is saved to an additional compile_commands.json file. Completely transparent for most purposes, but very useful if you want to open the source and existing build directory in KDevelop (which then won't rerun cmake with default settings).
  • cmake.install_rpath : a convenience function to construct CMAKE_INSTALL_RPATH in steps, working around the fact that cmake doesn't support incremental commandline variables.
  • lines 204-243 : ensure there's a compile_commands.json file, and generate a .macports.${subport}.configure.cmd file that summarises the configure details, as a more permanent reference than the information available through the logfile. There's been some discussion of adding this to "base", if that happens this feature will evidently disappear or be refactored.

comment:2 Changed 7 years ago by mkae (Marko Käning)

Cc: mojca michaelld added

Adding two cmake'ish devs for review.

comment:3 Changed 7 years ago by mkae (Marko Käning)

Cc: ryandesign added
Description: modified (diff)

As well as Ryan, of course.

comment:4 Changed 7 years ago by michaelld (Michael Dickens)

Does this fix address ticket #37732 ("provide option to disable CMAKE_OSX_ARCHITECTURES")? That would be really useful ...

comment:5 Changed 7 years ago by RJVB (René Bertin)

I don't think so, no, because I never had to deal with this issue myself.

It's unclear to me if there has been a proposal how to fix this in #37732. It seems the fix would have to be in CMake. But if it is a sufficient and acceptable workaround NOT to set CMAKE_OSX_ARCHITECTURES in the portgroup then that shouldn't be hard to implement.

Edit: as long as it's a flag to turn off setting the thing altogether, without compiler lookups

Last edited 7 years ago by RJVB (René Bertin) (previous) (diff)

comment:6 Changed 7 years ago by mojca (Mojca Miklavec)

That ticket is only asking for an option in the PortGroup, not for a fix in CMake. Sure, it would be nice if the bug in CMake was fixed, so that the flags would not proliferate to Fortran, but that's a different issue.

comment:7 Changed 7 years ago by RJVB (René Bertin)

Yeah, well, we usually aim for fixes, not workarounds, no? :)

As I said, it's easy enough to introduce such an option, in a way that nothing changes for most ports.

I remembered there was a project of making out-of-source builds the default, so I applied that too.

Version 0, edited 7 years ago by RJVB (René Bertin) (next)

comment:8 Changed 7 years ago by michaelld (Michael Dickens)

Cool. I'm all for creating a new cmake portgroup; this looks OK to me. Why don't we go ahead & get it in place, to allow cmake ports to test it out -- maybe add that this is a "work in progress for testing only" or something like that. Leave this ticket open & get feedback to tweak / correct any issues that come up. I like the changes implemented in this version, and if it works around a cmake issue, even better (thx, btw; I wasn't asking for you to do this fix, just enquiring if it did so).

comment:9 Changed 7 years ago by RJVB (René Bertin)

Great :)

Did you see my edit? Should we leave the CMAKE_OSX_ARCHITECTURES when a non-universal build is being done, or is it fine if the option disables all occasions? If you don't know the answer either that's fine too, it will be trivial to undo this part of the change should the need arise.

Marko, it seems you can go ahead and commit this. Once that's done we'll send out a message on macports-devel telling everyone about the immediate availability of this new portgroup version.

comment:10 Changed 7 years ago by mojca (Mojca Miklavec)

On the contrary. Unless I'm mistaken (I didn't check) CMAKE_OSX_ARCHITECTURES is actually needed to build universally, so it should stay.

I would only disable CMAKE_OSX_ARCHITECTURES on request. From what I know this is only a problem when Fortran is involved.

comment:11 Changed 7 years ago by michaelld (Michael Dickens)

Ditto what Mojca said: disable only on specific request.

comment:12 Changed 7 years ago by RJVB (René Bertin)

Which is what the patch does (cmake.set_osx_architectures is true by default; set it to false to disable CMAKE_OSX_ARCHITECTURES).

Let's just leave it at my current implementation then, and thus leave it up to port developers to add the appropriate arch flag(s) for their ports to build correctly. For a moment I had a doubt whether we needed to do something different in the case a port want to build, say, in 32bit on a 64bit host, but there's something to say for making that the port's responsibility.

comment:13 Changed 7 years ago by mkae (Marko Käning)

Status: newaccepted

OK, folks, can I commit the current state then?

comment:14 Changed 7 years ago by mkae (Marko Käning)

Clicking on the URL underlying the 'Port' data field "cmake" in the ticket header reveals a substantial ticket line for this port.

So, will we ignore all that for now and simply commit this new port group, so that devs can test this new version 1.1?

comment:15 in reply to:  13 ; Changed 7 years ago by larryv (Lawrence Velázquez)

Please remove the added copyright claim.

comment:16 Changed 7 years ago by larryv (Lawrence Velázquez)

Why the weird cmake.install_rpath function? cmake_install_rpath should just be an option so that it has the appending/prepending functionality that all other portfile options have. Then set

default configure.args {-DCMAKE_INSTALL_RPATH="[join ${cmake_install_rpath} ;]"}

to construct the path on the fly.

comment:17 in reply to:  15 ; Changed 7 years ago by mkae (Marko Käning)

Replying to larryv:

Please remove the added copyright claim.

Hmm, that’s interesting. Why would one not be allowed to append one's copyright, if one were the one who worked it out?

comment:18 in reply to:  17 Changed 7 years ago by larryv (Lawrence Velázquez)

It’s uncommon for individual contributors to add copyright attribution for additions this small.

comment:19 in reply to:  1 Changed 7 years ago by larryv (Lawrence Velázquez)

Replying to RJVB:

changes include:

  • use CMAKE_BUILD_TYPE=MacPorts. Inspired by Debian's own build type, this allows us to specify all compiler settings via the well-known configure.* commands and exported via the environment. If one of CMake's predefined types is used the corresponding standard options will be *appended* to our options, which will override notably the optimisation options. Some parsing of configure.cppflags, configure.cflags and configure.cxxflags is done to ensure this works as expected, in lines 145-200 .

This parsing code is very fragile. There’s almost certainly a better approach. Can you explain again why it is necessary at all? I am not very familiar with CMake.

  • lines 204-243 : ensure there's a compile_commands.json file, and generate a .macports.${subport}.configure.cmd file that summarises the configure details, as a more permanent reference than the information available through the logfile. There's been some discussion of adding this to "base", if that happens this feature will evidently disappear or be refactored.

Note that base augments several options before setting up the environment variables. For example, this:

puts ${fd} "CXXFLAGS=\"${configure.cxxflags}\""

will not include any -isysroot or -march flags that are added by base.

comment:20 Changed 7 years ago by RJVB (René Bertin)

This parsing code is very fragile. There’s almost certainly a better approach. Can you explain again why it is necessary at all? I am not very familiar with CMake.

I've been using this code for months now, and never had issues with it. You're probably right it's fragile but in practice it seems to work well enough, and it shouldn't drop any options which is probably the most important part.

You have much of the explanation right in the text you quoted. CMake defines 4 build types that each come with their own set of compiler options. Options set in the environment (CFLAGS, CXXFLAGS, etc) are not ignored, but they are prepended to the options CMake imposes. So if a port or user wants to build with -O3 but the build type wants -O2, you get -O3 -O2 which equals -O2. It's tricky to override this, unless you specify a custom build type which evidently doesn't come with predefined compiler options. Then you get only the options you set in the environment.

The brunt of the parsing however concerns configure.cppflags. This is done because CMake doesn't have an equivalent, it only has an option to define the include path (which has to be done in one go, not in incremental steps). So to be sure that all specified include paths are known to CMake this parser rewrites configure.cppflags so that it contains only the -I/foo and -I /foo options. All other options are moved to configure.*cflags and configure.*cxxflags.

Note that base augments several options before setting up the environment variables.

Meaning I should output $::env(CXXFLAGS), or does it not yet have its final value in the pre-configure? Not that this is crucial, but it'd be nice to get it right.

comment:21 Changed 7 years ago by RJVB (René Bertin)

(I'm having difficulties finding the right button to submit comments, so here's a reply that was still not posted :-/)

I found my additions significant enough to bother signing them.

Replying to larryv:

Why the weird cmake.install_rpath function?

Define weird?! I have never found any documentation how to create custom variables with -append, -prepend etc. functionality, so I rolled my own implementation. I'm sorry (for you) if you find that weird...

cmake_install_rpath should just be an option so that it has the appending/prepending functionality that all other portfile options have. Then set

default configure.args {-DCMAKE_INSTALL_RPATH="[join ${cmake_install_rpath} ;]"}

to construct the path on the fly.

I must be doing something weird^H\^H\^H\^Hrong then:

options cmake.out_of_source cmake.build_dir cmake.set_osx_architectures cmake_install_rpath
#snip
default cmake_install_rpath ${prefix}/lib

default configure.args {-DCMAKE_INSTALL_RPATH="[join ${cmake_install_rpath} ;]"}

configure.args-append \
                    -DCMAKE_VERBOSE_MAKEFILE=ON \
                    -DCMAKE_COLOR_MAKEFILE=ON \
                    -DCMAKE_BUILD_TYPE=MacPorts \
                    -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON \
                    -DCMAKE_INSTALL_NAME_DIR=${prefix}/lib \
                    -DCMAKE_SYSTEM_PREFIX_PATH="${prefix}\;/usr" \
                    -DCMAKE_MODULE_PATH=${cmake_share_module_dir} \
                    -DCMAKE_FIND_FRAMEWORK=LAST \
                    -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
                    -Wno-dev

in a test Portfile:

PortGroup cmake 1.1
#snip
cmake_install_rpath-prepend ${qt_libs_dir}
ui_msg "cmake_install_rpath=${cmake_install_rpath}"
#snip
configure.args-append -Dfoo
#snip
ui_msg "configure.args=${configure.args}"

prints

cmake_install_rpath=/opt/local/libexec/qt5/lib /opt/local/lib
configure.args=-DCMAKE_INSTALL_RPATH="/opt/local/lib" -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_COLOR_MAKEFILE=ON -DCMAKE_BUILD_TYPE=MacPorts -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON -DCMAKE_INSTALL_NAME_DIR=/opt/local/lib {-DCMAKE_SYSTEM_PREFIX_PATH="/opt/local;/usr"} -DCMAKE_MODULE_PATH=/opt/local/share/cmake/Modules -DCMAKE_FIND_FRAMEWORK=LAST -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -Wno-dev -Dfoo

in other words, the -prepend feature works, but the default configure.args statement doesn't. I also don't see how it could work and expand the final value of cmake_install_rpath as a default/initial value for configure.arg when there's no guarantee about the order in which cmake_install_rpath and configure.args get elements appended or prepended.

The only thing I could imagine is to append [join ${cmake_install_rpath}] in a pre-configure block,but even then you cannot be certain that the user will not add one of those too that touches cmake_install_rpath.

Last edited 7 years ago by RJVB (René Bertin) (previous) (diff)

Changed 7 years ago by RJVB (René Bertin)

Attachment: cmake-PortGroup.diff added

comment:22 Changed 7 years ago by mkae (Marko Käning)

Above test port file contains cmake_install_rpath.

Did you mean cmake.install_rpath or am I misunderstanding something there?

comment:23 Changed 7 years ago by mkae (Marko Käning)

Could you perhaps for testing attach a Portfile for some application which would make use of this port group?

comment:24 Changed 7 years ago by RJVB (René Bertin)

Did you mean cmake.install_rpath or am I misunderstanding something there?

Since I got the option to work to satisfaction I gave it the same name as the old function, to be in line with the other options.

QtCurve-qt5 +qtonly uses the feature: https://github.com/RJVB/macstrop/tree/master/kde/QtCurve

I have verified that the setting is handed off to CMake correctly, I haven't rebuilt the port with that variant. I have however just tested the feature elsewhere, and it works as intended.

comment:25 Changed 7 years ago by mkae (Marko Käning)

Okay, will then commit ASAP. :-)

comment:26 Changed 7 years ago by mkae (Marko Käning)

Committed in r154368.

Please try using the new version 1.1 of PortGroup cmake!

I'll leave this ticket open until I get (positive) feedback from some of you.

comment:27 Changed 7 years ago by mkae (Marko Käning)

Did anyone test this?

comment:28 Changed 7 years ago by mkae (Marko Käning)

Resolution: fixed
Status: acceptedclosed

OK, I consider this ticket closed now.

comment:29 in reply to:  1 Changed 7 years ago by larryv (Lawrence Velázquez)

Replying to RJVB:

  • lines 204-243 : ensure there's a compile_commands.json file, and generate a .macports.${subport}.configure.cmd file that summarises the configure details, as a more permanent reference than the information available through the logfile. There's been some discussion of adding this to "base", if that happens this feature will evidently disappear or be refactored.

I’m not convinced that this should remain. Since it’s created in workpath, it still gets trashed on port clean, so I don’t see how it’s any more permanent than the logfile.

Note: See TracTickets for help on using tickets.