Use "gmake" command in custommake plugin on FreeBSD. Fixes unit test.
ClosedPublic

Authored by arrowd on Aug 10 2018, 1:51 PM.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
arrowd created this revision.Aug 10 2018, 1:51 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptAug 10 2018, 1:51 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
arrowd requested review of this revision.Aug 10 2018, 1:51 PM

Yay for more unit test fixes :)

Though... why does this change fix the unit test exactly? Without knowing more details, I am surprised that one cannot use the normal "make". "gmake" does not occur elsewhere as string, so why is this needed here, and why not elsewhere as well (not sure if there are other direct calls of the "make" tool).

Does the call rely on some GNU make features? If so, that should be added at least as comment in the code, so it is clear why plain "make" does not work.
Also, is "gmake" part of the normal FreeBSD system? Or would we need to check first this exists in the runtime?

plugins/custommake/makefileresolver/makefileresolver.cpp
98–104

Type of make could be QLatin1String as well, given below in the concatenation QStringBuilder happily deals with QLatin1Strings, saves us one conversion/malloc at runtime.

100

The reassignment looks strange, IMHO would be better to do

#ifdef Q_OS_FREEBSD
// on FreeBSD gmake is needed because foobar
const QLatin1String makeExe("gmake");
#else
const QLatin1String makeExec("make");
#endif

Yay for more unit test fixes :)

Though... why does this change fix the unit test exactly? Without knowing more details, I am surprised that one cannot use the normal "make".

The make coming with FreeBSD is BSD one, it doesn't support some GNU extensions.

"gmake" does not occur elsewhere as string, so why is this needed here, and why not elsewhere as well (not sure if there are other direct calls of the "make" tool).

Haven't found other "make" uses. And it fixed the test, after all.

Does the call rely on some GNU make features? If so, that should be added at least as comment in the code, so it is clear why plain "make" does not work.

MakeFileResolver::resolveIncludePathInternal calls make with --no-print-directory flag, which isn't supported by BSD make.

Also, is "gmake" part of the normal FreeBSD system? Or would we need to check first this exists in the runtime?

It is not, but what we can do if there is no gmake? Lets just require packagers to install gmake along with KDevelop.

Does the call rely on some GNU make features? If so, that should be added at least as comment in the code, so it is clear why plain "make" does not work.

MakeFileResolver::resolveIncludePathInternal calls make with --no-print-directory flag, which isn't supported by BSD make.

I see, so that should be documented then in the code, please.

Also, is "gmake" part of the normal FreeBSD system? Or would we need to check first this exists in the runtime?

It is not, but what we can do if there is no gmake? Lets just require packagers to install gmake along with KDevelop.

For one, the plugin could disable itself, like others do if their runtime dep is not found :) Which though should be improved in general, as this happens silently and the user is left clueless.
In any case, MakeFileResolver::executeCommand() could get some error handling incl. a message box if the execution fails

To make packagers & people building kdevelop aware of that dep, best copy FindCppcheck.cmake to some Findgmake.cmake or similar and adapt to what is needed with gmake in case of FreeBSD, then add to plugins/custommake/CMakeLists.txt this:

if(CMAKE_SYSTEM_NAME STREQUAL "FreeBSD")
find_package(gmake QUIET)
set_package_properties(gmake PROPERTIES
    DESCRIPTION "The GNU make"
    PURPOSE "Required by the custommake plugin"
    TYPE RUNTIME
)
endif()
arrowd updated this revision to Diff 39439.Aug 11 2018, 9:44 AM

Fix the test another way.

arrowd updated this revision to Diff 39440.Aug 11 2018, 9:48 AM
arrowd marked 2 inline comments as done.

Address comments.

It turned out that BSD make just don't need that flag, so add it conditionally.

Would it still make sense on FreeBSD to try findExecutable("gmake") first and use that if found?

kossebau accepted this revision.Aug 13 2018, 8:03 PM

It turned out that BSD make just don't need that flag, so add it conditionally.

Okay.

Would it still make sense on FreeBSD to try findExecutable("gmake") first and use that if found?

Not sure. If the normal make of FrreeBSD does anything that is needed here to estimate the include dirs, things should be fine as it is.

plugins/custommake/makefileresolver/makefileresolver.cpp
99

Please add a comment saying why this is needed, e.g.

// GNU make implicitly enables "-w" for sub-makes, we don't want that
This revision is now accepted and ready to land.Aug 13 2018, 8:03 PM
This revision was automatically updated to reflect the committed changes.