Bug 202516 - [exp-run] Update CMake to 3.3.1.
Summary: [exp-run] Update CMake to 3.3.1.
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Antoine Brodin
URL:
Keywords:
Depends on: 202618 202798 202905
Blocks:
  Show dependency treegraph
 
Reported: 2015-08-20 13:17 UTC by Raphael Kubo da Costa
Modified: 2015-09-07 12:24 UTC (History)
4 users (show)

See Also:


Attachments
patch for converters/libiconv and Mk/Uses/iconv.mk (2.32 KB, patch)
2015-08-31 10:50 UTC, Tijl Coosemans
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa freebsd_committer 2015-08-20 13:17:34 UTC
kde@ would like to request an exp-run for devel/cmake, devel/cmake-gui and devel/cmake-modules.

The patch is available at https://people.freebsd.org/~rakuco/exp-runs/cmake-3.3.1-v1.patch. Please note that the ports tree must be at revision 394861 or later.
Comment 1 Antoine Brodin freebsd_committer 2015-08-21 08:05:59 UTC
First results with 8 errors and a few hundreds ports skipped:

http://package23.nyi.freebsd.org/build.html?mastername=101amd64-default-PR202516&build=2015-08-20_17h25m48s

Other problems found:

- audio/hydrogen links against libarchive.so.13 but doesn't register a dependency on it

- databases/mysql56-client links against libedit from base (needs to be converted to USES+=libedit ?)

- databases/percona56-client links against libedit from base (needs to be converted to USES+=libedit ?)

For the iconv errors,  you may ask help from tijl@,  it seems ICONV_SECOND_ARGUMENT_IS_CONST is defined when it shouldn't be   (see also bug #199099 )

I will be afk 1 week.
Comment 2 Tijl Coosemans freebsd_committer 2015-08-22 13:02:25 UTC
10.1 is special because /usr/include/iconv.h still has the const keyword in the iconv(3) declaration.  This has been removed in 10.2 for POSIX compliance and the ports tree only really supports without const because supporting both with and without in all ports is too much work.  To make this work on 10.1 USES=iconv adds a build dependency on converters/libiconv so ports can use its iconv.h instead of the base system iconv.h.  This requires -I${LOCALBASE}/include.  The problem you see in libstreamanalyzer is because make configure does not use -I${LOCALBASE}/include (and detects const) while make build does (and doesn't want const).

I think the best solution is for USES=iconv to add -DICONV_SECOND_ARGUMENT_IS_CONST:BOOL=FALSE to CMAKE_ARGS until 10.1 is EoL.
Comment 3 Raphael Kubo da Costa freebsd_committer 2015-08-22 14:37:32 UTC
There's an alternative solution I proposed in a comment in bug 186704. Right now both libstreamanalyzer and libstreams force the use of base's iconv due to r344139, and in addition to that I think CMake 3.2's calls to find_library() and find_path() end up preferring the files in base (I need to double-check the latter).

Things work as expected on 10.1 if I revert r344139 and make Uses/iconv.mk include Uses/localbase.mk in the 10.1 case, as the latter sets CMAKE_PREFIX_PATH and CMake will then give preference to the files in $LOCALBASE instead. Does that sound reasonable to you? Would you like to see a patch first?
Comment 4 Tijl Coosemans freebsd_committer 2015-08-22 16:12:09 UTC
(In reply to Raphael Kubo da Costa from comment #3)
When you revert ports r344139 both ports will link to libiconv when it happens to be installed without registering a dependency on it.  It's caused by FindIconv.cmake which prefers libiconv over libc.  Other ports like audio/tagutil patch that file instead of using CMAKE_ARGS.  Maybe using such a patch here would make the intent clearer.

Localbase.mk should be deleted in my opinion.  It's not safe to add -I and -L flags to *FLAGS like it does because the order of -I and -L flags on the command line is important.  -I${LOCALBASE}/include and -L${LOCALBASE}/lib normally need to be added after any -I and -L flags from upstream.  For GNU configure based ports this means adding them to CPPFLAGS/LIBS, not CFLAGS/LDFLAGS because these appear too early on the command line.  Currently very few ports have been exposed to localbase.mk so this problem doesn't seem to have come up yet, but including it from iconv.mk would certainly cause problems.
Comment 5 Raphael Kubo da Costa freebsd_committer 2015-08-22 22:46:08 UTC
I still haven't fully understood what the expected behavior on 10.1 is supposed to be. For example, looking at the build logs with CMake 3.2.2, libstreamanalyzer links against base iconv (in libc) but uses iconv.h from ports; wv2 uses both libiconv.so and iconv.h from ports.

Since the code in iconv.mk forces a build-dependency on converters/iconv on 10.1 (and DragonFly, and 9.x etc), I would have expected the desired behavior to be to use header and library from ports.
Comment 6 Raphael Kubo da Costa freebsd_committer 2015-08-22 22:51:14 UTC
Additionally, I think just setting ICONV_SECOND_ARGUMENT_IS_CONST to FALSE is not enough. The fact that it's being set to TRUE in several cases is a consequence of find_path(iconv.h) now defaulting to /usr/include/iconv.h instead of the ports one because find_path(), find_library() and find_file() now take $PATH into consideration when looking for things, and /usr generally comes before /usr/local.

Possible fixes/workarounds involve manually passing ICONV_INCLUDE_DIR via CMAKE_ARGS (but different ports might use different names for the variable), or setting CMAKE_PREFIX_PATH (which localbase.mk currently does).
Comment 7 Tijl Coosemans freebsd_committer 2015-08-23 09:09:45 UTC
(In reply to Raphael Kubo da Costa from comment #5)
The intention is for all ports to use base system iconv(3) whenever possible so we have the following:

if !exists(/usr/include/iconv.h) (e.g. FreeBSD 9) || iconv:wchar_t || iconv:translit

either there is no base system iconv(3) or GNU extensions are needed so use converters/libiconv (both iconv.h and libiconv so LIB_DEPENDS)

else

base system has iconv(3) so let's use it, but...

if DragonFly || FreeBSD 10.1

/usr/include/iconv.h is broken so let's use iconv.h (but not libiconv so BUILD_DEPENDS) from converters/libiconv with -DLIBICONV_PLUG which makes it pretend to be /usr/include/iconv.h

endif
endif


So wv2 is misbehaving.  It also needs a patch for FindIconv.cmake.
Comment 8 Tijl Coosemans freebsd_committer 2015-08-23 09:14:32 UTC
(In reply to Raphael Kubo da Costa from comment #6)
For GNU configure based ports iconv.mk defines helper variables ICONV_CONFIGURE_ARG and ICONV_CONFIGURE_BASE that they can add to their CONFIGURE_ARGS.  Maybe there should be an ICONV_CMAKE_ARGS variable that CMake based ports can then add to CMAKE_ARGS if needed.  ICONV_CMAKE_ARGS only needs to set ICONV_INCLUDE_DIR and ICONV_LIBRARIES.  ICONV_SECOND_ARGUMENT_IS_CONST should indeed be correct when ICONV_INCLUDE_DIR is correct.
Comment 9 Raphael Kubo da Costa freebsd_committer 2015-08-23 22:00:10 UTC
(In reply to Tijl Coosemans from comment #8)
> Maybe there should be an ICONV_CMAKE_ARGS variable that CMake based ports can
> then add to CMAKE_ARGS if needed. ICONV_CMAKE_ARGS only needs to set
> ICONV_INCLUDE_DIR and ICONV_LIBRARIES. ICONV_SECOND_ARGUMENT_IS_CONST should
> indeed be correct when ICONV_INCLUDE_DIR is correct.

The problem with ICONV_CMAKE_ARGS is that ports use different variable names. For example, libstreamanalyzer uses ICONV_INCLUDE_DIR and ICONV_LIBRARIES, whereas chinese/fcitx-googlepinyin uses LIBICONV_{INCLUDE_DIR,LIBRARIES}.

We could either set both, or do that in each port that depends on both iconv and cmake (I haven't checked if there are many), as I don't think we can automate things with CMAKE_PREFIX_PATH if the idea is to use the library from base and the header from ports.
Comment 10 Tijl Coosemans freebsd_committer 2015-08-26 11:26:05 UTC
(In reply to Raphael Kubo da Costa from comment #9)
There are already two helper variables for GNU configure ports so I don't mind adding two for cmake, but one variable which sets both options is fine too I think.  Then you don't even need a helper variable.  You can add the definitions directly to CMAKE_ARGS.  Ports that use uncommon names can set their own CMAKE_ARGS or use patches.

To prevent this from getting worse in the future FindIconv.cmake should be added to cmake though.  It needs to handle the following three cases:

- iconv.h and iconv() provided by libc (e.g. in /usr)
- iconv.h and iconv() provided by libiconv  (e.g. in /usr/local)
- both of the above are available, prefer libc

Unless LIBICONV_PLUG is defined, the iconv.h from libiconv renames all functions to something only libiconv provides (iconv() becomes libiconv(), iconv_open() becomes libiconv_open() etc.).  So, without LIBICONV_PLUG this header enforces the use of libiconv, with LIBICONV_PLUG the intention is to use libc (but libiconv can still be used as a fall back).  This means that after having found iconv.h FindIconv.cmake should look for the library by compiling+linking a test program that includes <iconv.h> and calls iconv().  This test should try libc first and then libiconv (and other names for it).
Comment 11 Raphael Kubo da Costa freebsd_committer 2015-08-30 22:34:39 UTC
I agree that having a FindIconv.cmake would be nice, but it wouldn't solve the problems immediately as those ports shipping their own versions would still end up using them anyway.

This past few days I've been removing USES=iconv from some ports that don't need it and have taken a look at the situation for the rest of those who use CMake and iconv. I am considering the approach of adding something like this to Uses/iconv.mk:

  CMAKE_ARGS+= -DICONV_INCLUDE_DIR=${ICONV_INCLUDE_PATH} \
               -DICONV_LIBRARY=${ICONV_LIB_PATH} \
               -DICONV_LIBRARIES=${ICONV_LIB_PATH} \
               -DLIBICONV_INCLUDE_DIR=${ICONV_INCLUDE_PATH} \
               -DLIBICONV_LIBRARY=${ICONV_LIB_PATH} \
               -DLIBICONV_LIBRARIES=${ICONV_LIB_PATH}

with ICONV_INCLUDE_PATH being either /usr/include or ${LOCALBASE}/include and ICONV_LIB_PATH either /usr/lib/libc.so or ${LOCALBASE}/lib/libiconv.so, depending on the rest of the logic in iconv.mk. It also allows us to get rid of some of the FindIconv.cmake patches we currently have in the ports tree. Would that approach be acceptable to you? If so, I'm going to finish my tests and file another exp-run request for it, and the intention is to land it before this bug.
Comment 12 Tijl Coosemans freebsd_committer 2015-08-31 10:50:29 UTC
Created attachment 160556 [details]
patch for converters/libiconv and Mk/Uses/iconv.mk

On FreeBSD libiconv currently exports both iconv*() and libiconv*() ((lib)iconv(), (lib)iconv_open() and (lib)iconv_close()), but this is for historic reasons and should not really be necessary.  The upstream libiconv only exports libiconv*().

This patch for converters/libiconv removes iconv*() from libiconv.  It might simplify the way a fixed FindIconv.cmake can find the right library because both /usr/include/iconv.h and /usr/local/include/iconv.h+LIBICONV_PLUG will then force the use of libc (iconv*()) while /usr/local/include/iconv.h without LIBICONV_PLUG will force the use of libiconv (libiconv*()).

This patch also includes changes to Mk/Uses/iconv.mk so it sets CMAKE_ARGS when necessary.
Comment 13 Antoine Brodin freebsd_committer 2015-08-31 16:57:51 UTC
So,  should I do a first exp-run with the iconv patch?
Comment 14 Tijl Coosemans freebsd_committer 2015-08-31 17:47:13 UTC
(In reply to Antoine Brodin from comment #13)
I'd like to see Raphael confirm that it fixes his cases and then I think it can be included in the cmake exp-run.
Comment 15 Raphael Kubo da Costa freebsd_committer 2015-08-31 20:51:08 UTC
I've been testing a patch similar to the one Tijl proposed, and it seems to have been working fine. I've filed bug 202798 to ask for an exp-run for that patch as well as some related changes (I'm not sure if the change in converters/libiconv wil break something), and I'd like to land that one before carrying on with this CMake exp-run.
Comment 16 Antoine Brodin freebsd_committer 2015-09-03 13:31:18 UTC
Hi,

Is this patch still the one that has to be tested: https://people.freebsd.org/~rakuco/exp-runs/cmake-3.3.1-v1.patch ?
Comment 17 Raphael Kubo da Costa freebsd_committer 2015-09-03 15:49:49 UTC
Yes, please. Just make sure the ports tree is at r395972 or later.
Comment 19 Raphael Kubo da Costa freebsd_committer 2015-09-03 16:31:16 UTC
Grrmbl, I forgot I had landed a patch to devel/cmake a few days ago. Sorry about that.

https://people.freebsd.org/~rakuco/exp-runs/cmake-3.3.1-v2.patch should apply correctly.
Comment 21 Antoine Brodin freebsd_committer 2015-09-04 05:33:28 UTC
Other problems still found:

- audio/hydrogen links against libarchive.so.13 but doesn't register a dependency on it

- databases/mysql56-client links against libedit from base (needs to be converted to USES+=libedit ?)
Comment 22 Raphael Kubo da Costa freebsd_committer 2015-09-04 18:23:45 UTC
(In reply to Antoine Brodin from comment #21)
> Other problems still found:
> 
> - audio/hydrogen links against libarchive.so.13 but doesn't register a
> dependency on it

Are you sure? I thought it was just using libarchive from base all the time; if that's not the case I can add a USES=libarchive there.

> - databases/mysql56-client links against libedit from base (needs to be
> converted to USES+=libedit ?)

Still waiting for feedback in bug 202618. Shall I just land it?
Comment 23 Raphael Kubo da Costa freebsd_committer 2015-09-04 18:25:07 UTC
The owncloudclient failure is interesting in the sense that it's caused by the same behavior change that was making CMake previously choose base iconv over the ports one.

I'm thinking of just setting CMAKE_PREFIX_PATH in the port, but it might be good to make Mk/bsd.openssl.mk use Uses/localbase.mk in the future.
Comment 24 Antoine Brodin freebsd_committer 2015-09-04 18:57:26 UTC
(In reply to Raphael Kubo da Costa from comment #22)

With what is on pkg.freebsd.org right now:

% pkg rquery %B hydrogen
libsndfile.so.1
liblrdf.so.2
libjack.so.0
libarchive.so.13
libQtXmlPatterns.so.4
libQtXml.so.4
libQtNetwork.so.4
libQtGui.so.4
libQtCore.so.4

% pkg rquery %do hydrogen
x11-toolkits/qt4-gui
textproc/qt4-xml
textproc/liblrdf
net/qt4-network
devel/qt4-qt3support
devel/qt4-corelib
devel/desktop-file-utils
databases/qt4-sql
audio/libsndfile
audio/ladspa
audio/jack
Comment 25 Antoine Brodin freebsd_committer 2015-09-04 19:00:34 UTC
(In reply to Raphael Kubo da Costa from comment #22)

bug #202618 should be landed at the same time or before the cmake update
Comment 26 commit-hook freebsd_committer 2015-09-07 12:10:27 UTC
A commit references this bug:

Author: rakuco
Date: Mon Sep  7 12:09:56 UTC 2015
New revision: 396266
URL: https://svnweb.freebsd.org/changeset/ports/396266

Log:
  Update CMake to 3.1.1.

  Release notes for the 3.3 series:
  http://www.cmake.org/cmake/help/v3.3/release/3.3.html

  This update took longer than expected because of a behavior change in CMake:
  now calls to find_library(), find_path() etc will take the $PATH environment
  variable into consideration, which in practice means that it will prefer
  libraries in base instead of those in ports when both versions are available.
  r395972 is an example of the groundwork that had to be done before landing this
  patch.

  - deskutils/owncloudclient: When OpenSSL from ports is to be used, make sure to
    pass ${LOCALBASE} as $CMAKE_PREFIX_PATH, otherwise it will use the version in
    base (see above) and fail on 9.x.
  - math/cgal: Import upstream patch to fix the configuration process with CMake
    3.3.x.

  PR:		202516

Changes:
  head/deskutils/owncloudclient/Makefile
  head/devel/cmake/Makefile
  head/devel/cmake/distinfo
  head/devel/cmake/files/patch-Modules_FindwxWidgets.cmake
  head/devel/cmake/files/patch-git_c775072a
  head/devel/cmake/pkg-plist
  head/devel/cmake-gui/Makefile
  head/devel/cmake-modules/pkg-plist
  head/math/cgal/files/patch-src_CMakeLists.txt
Comment 27 Raphael Kubo da Costa freebsd_committer 2015-09-07 12:24:02 UTC
Landed, at last! Thanks everyone.