Page MenuHomeFreeBSD

Update CMake to 3.9
ClosedPublic

Authored by adridg on Sep 9 2017, 9:02 PM.

Details

Summary

Update CMake to 3.9; merge devel/cmake-modules with devel/cmake.
Patch ports that have issues with CMake 3.9. Mark graphics/nomacs
as BROKEN, since this version really isn't going to build with
CMake 3.9 -- there is another PR with an update to graphics/nomacs
that updates and fixes that.

Test Plan

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11573
Build 11926: arc lint + arc unit

Event Timeline

adridg created this revision.Sep 9 2017, 9:02 PM
tcberner edited the test plan for this revision. (Show Details)Sep 9 2017, 9:35 PM
tcberner added inline comments.Sep 9 2017, 9:49 PM
devel/cmake/Makefile
67–69

This looks like something that could use an explanation :)

adridg updated this revision to Diff 32865.Sep 9 2017, 10:26 PM

Small cleanups in devel/cmake/Makefile and explain the .NoDartCoverage rm

tcberner accepted this revision.Sep 9 2017, 10:31 PM

Given that the exp-run was fine, I think this should be good to go.
@rakuco do you think it's fine to just mark graphics/nomacs broken for now? -- I did not yet receive any response on the update pr for it, and as it is a major update, I would like some response ore wait for some timeout.

This revision is now accepted and ready to land.Sep 9 2017, 10:31 PM

The patch looks mostly good; I've added some inline questions and comments due to the amount of ports being changed in this update. In general, I'm asking for clarification on whether some changes are really related to this change and also asking for some context in the patches being added, otherwise we end up in a very confusing situation in the future.

audio/sayonara/files/patch-CMakeLists.txt
2

When a patch is upstreamed, I tend to put the commit header to indicate the hash and the commit message.

audio/sayonara/files/patch-uifiles.patch
2

It'd be good to add an explanation about why this is needed.

cad/brlcad/Makefile
41 ↗(On Diff #32865)

This looks unrelated, can you land this separately?

databases/kdb/files/patch-src_CMakeLists.txt
2

Was this upstreamed/obtained from upstream?

devel/aws-sdk-cpp/Makefile
23 ↗(On Diff #32865)

Is this caused by the CMake update? In this case, I tend have a separate CXXFLAGS+= -Wno-c11-extensions block with a comment explaining why this is added.

devel/cmake/files/patch-Modules_Platform_FreeBSD-CXX.cmake
2

Can you explain why this is needed and whether this was upstreamed?

devel/jsoncpp/Makefile
6 ↗(On Diff #32865)

Can this be landed before this update?

emulators/ppsspp-qt5/Makefile
3 ↗(On Diff #32865)

Why is this being bumped?

emulators/ppsspp/files/patch-system-libpng16
59

I'd avoid this unnecessary change here that only moved this hunk below.

finance/kmymoney-kde4/Makefile
5 ↗(On Diff #32865)

I don't see the need to bump PORTREVISION.

finance/kmymoney-kde4/files/patch-kmymoney_dialogs_settings_CMakeLists.txt
2

Can you also add if this was obtained from upstream or whether this will be upstreamed?

multimedia/avidemux/files/patch-avidemux_qt4_ADM__jobs_src_ADM__jobControl.cpp
1 ↗(On Diff #32865)

Was this upstreamed, or imported from upstream? It'd be good to explain if this is caused by CMake building with a different C++ standard (otherwise this should be landed independently from this update).

security/greenbone-security-assistant8/Makefile
19 ↗(On Diff #32865)

It looks like these changes can be landed separately from the CMake update. Can you do that before committing this?

Oh, and this needs a PORTREVISION bump due to new the files being installed and the new dependency on libgpg-error.

security/openconnect-gui/Makefile
6 ↗(On Diff #32865)

Why is PORTREVISION being bumped here?

security/openconnect-gui/files/patch-CMake_Includes_git__revision__main.cmake
2

Can you add some context and explain why this is needed and whether this was fetched from upstream/we're upstreaming it?

textproc/zorba/pkg-plist
207 ↗(On Diff #32865)

Is this really needed? The directory should be removed automatically.

x11/kdelibs4/Makefile
6 ↗(On Diff #32865)

There's no need to bump PORTREVISION, the patch you're adding does not change any of the files this port installs.

tcberner added inline comments.Sep 14 2017, 5:09 AM
finance/kmymoney-kde4/files/patch-kmymoney_dialogs_settings_CMakeLists.txt
2

Upstream's version was ported to KF5, and is not affected by this -- I bumped the revision, beceause this CMake code seems to haven been clearly broken before, and I'm not sure whether the package might have changed therefore.

multimedia/avidemux/files/patch-avidemux_qt4_ADM__jobs_src_ADM__jobControl.cpp
1 ↗(On Diff #32865)

sorry, that's on me :) -- it should not be part of this.

adridg marked 11 inline comments as done.Sep 14 2017, 1:00 PM
adridg added inline comments.
cad/brlcad/Makefile
41 ↗(On Diff #32865)

It was in response to stage-qa in poudriere, but is indeed unrelated. Maintainer should be informed.

emulators/ppsspp-qt5/Makefile
3 ↗(On Diff #32865)

Similar to the kmymoney case -- patching cmakelists, suggests that *something* might change. But there is no functional change, indeed.

adridg marked 4 inline comments as done.Sep 14 2017, 1:07 PM
adridg added inline comments.
security/greenbone-security-assistant8/Makefile
19 ↗(On Diff #32865)

The new dep comes from stage-qa; the new files only happen with CMake 3.9, so can't really be done separately.

textproc/zorba/pkg-plist
207 ↗(On Diff #32865)

stage-qa said so

adridg updated this revision to Diff 33065.Sep 14 2017, 1:15 PM
adridg marked an inline comment as done.

Drop extraneous bits, address rakuco's comments

This revision now requires review to proceed.Sep 14 2017, 1:15 PM
adridg marked 2 inline comments as done.Sep 14 2017, 1:25 PM
adridg added inline comments.
finance/kmymoney-kde4/files/patch-kmymoney_dialogs_settings_CMakeLists.txt
2

This has been patched (differently) for the upcoming kmymoney 4.8.1; discussed with the maintainer.

rakuco added inline comments.Sep 14 2017, 2:34 PM
audio/sayonara/files/patch-uifiles.patch
2

Any specific reason why the other new sayonara patches fixing the include paths weren't included here (pun unintended)?

devel/aws-sdk-cpp/Makefile
25 ↗(On Diff #33065)

It's still unclear whether this is needed or not; if it is not, you can just remove this line, otherwise you should re-enable it and add a comment with an explanation.

net/remmina/pkg-plist
100 ↗(On Diff #33065)

This looks unrelated to the update (and the plist is not sorted and the port needs a PORTREVISION bump).

security/greenbone-security-assistant8/Makefile
19 ↗(On Diff #32865)

I've just run poudriere testbuild on this port with the CMake version in ports and got both the stage-qa warning and the plist failures fixed here. The official Poudriere logs (e.g. http://beefy9.nyi.freebsd.org/data/latest-per-pkg/greenbone-security-assistant8/6.0.12_1/110amd64-default.log) also indicate the plist failures are already present.

This should really be taken care of separately.

mat added inline comments.Sep 14 2017, 3:04 PM
devel/cmake/files/patch-Modules_Platform_FreeBSD-CXX.cmake
2

The c++ compiler is called CXX, not CC.

$ make -V CC -V CXX
cc
c++

We went to great trouble removing all hardcoded gcc values, please do not add more :-)

adridg marked 9 inline comments as done.Sep 14 2017, 8:34 PM
adridg added inline comments.
devel/aws-sdk-cpp/Makefile
25 ↗(On Diff #33065)

It rebuild on amd64 fine without that flag, so I've dropped it.

devel/cmake/files/patch-Modules_Platform_FreeBSD-CXX.cmake
2

You misunderstand: prefer /usr/bin/c++ over /usr/bin/CC. That is why the patch is setting CMAKE_CXX_COMPILER_NAMES.

net/remmina/pkg-plist
100 ↗(On Diff #33065)

Reverted. It's probably because stage-qa or portlint complains about it.

adridg updated this revision to Diff 33087.Sep 14 2017, 8:35 PM
adridg marked 3 inline comments as done.

Update patch -- remove some unrelated changes

rakuco accepted this revision.Sep 14 2017, 8:41 PM

lgtm
plase make sure you upstream the CC->c++ change to CMake.

This revision is now accepted and ready to land.Sep 14 2017, 8:41 PM
adridg closed this revision.Sep 19 2017, 7:59 AM

This was committed in r449853