Bug 202516
| Summary: | [exp-run] Update CMake to 3.3.1. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Raphael Kubo da Costa <rakuco> | ||||
| Component: | Ports Framework | Assignee: | Antoine Brodin <antoine> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | CC: | FreeBSD, kde, portmgr, tijl | ||||
| Priority: | --- | ||||||
| Version: | Latest | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Bug Depends on: | 202618, 202798, 202905 | ||||||
| Bug Blocks: | |||||||
| Attachments: |
|
||||||
|
Description
Raphael Kubo da Costa
2015-08-20 13:17:34 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. 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.
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? (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. 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. 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). (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. (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. (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. (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). 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.
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.
So, should I do a first exp-run with the iconv patch? (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. 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. 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 ? Yes, please. Just make sure the ports tree is at r395972 or later. There seems to be a problem: http://package23.nyi.freebsd.org/data/93i386-default-PR202516/2015-09-03_16h08m52s/logs/errors/cmake-modules-3.3.1.log 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. Exp-run results: 10.2 amd64: http://package18.nyi.freebsd.org/build.html?mastername=102amd64-default-PR202516&build=2015-09-03_16h33m27s 10.1 i386: http://package22.nyi.freebsd.org/build.html?mastername=101i386-default-PR202516&build=2015-09-04_05h21m39s 9.3 i386: http://package23.nyi.freebsd.org/build.html?mastername=93i386-default-PR202516&build=2015-09-03_16h33m33s 1 new failure on all archs: math/cgal 2 skipped on all archs due to this failure: databases/pgrouting databases/sfcgal 1 new failure specific to 9.3: deskutils/owncloudclient Failure logs: http://package18.nyi.freebsd.org/data/102amd64-default-PR202516/2015-09-03_16h33m27s/logs/errors/cgal-4.6.log http://package23.nyi.freebsd.org/data/93i386-default-PR202516/2015-09-03_16h33m33s/logs/errors/owncloudclient-1.8.4.log 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 ?) (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? 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. (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 (In reply to Raphael Kubo da Costa from comment #22) bug #202618 should be landed at the same time or before the cmake update 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 Landed, at last! Thanks everyone. |