Fix CI failure on the KDE build server
AbandonedPublic

Authored by karthikp on Aug 17 2019, 11:50 PM.

Details

Reviewers
bcooksley
Group Reviewers
KDE Games
Summary

This fixes the build error caused by the presence of a whitespace character in the path used to build this project on https://build.kde.org/. The source of the whitespace is the environment variable JOB_BASE_NAME (set to 'kf5-qt5 SUSEQt5.12'). However, it would be a good idea to fix this in the cmake code, too.

Test Plan

Tested by building in a path with and without spaces. The CMake error seen when building in a path with spaces goes away when this patch is applied.

Diff Detail

Repository
R387 Kajongg
Lint
Lint Skipped
Unit
Unit Tests Skipped
karthikp created this revision.Aug 17 2019, 11:50 PM
Restricted Application added a reviewer: KDE Games. · View Herald TranscriptAug 17 2019, 11:50 PM
Restricted Application added a subscriber: kde-games-devel. · View Herald Transcript
karthikp requested review of this revision.Aug 17 2019, 11:50 PM

The space is principally in the name to expose failures such as this one (because spaces in paths created by users are common, and are almost guaranteed to be included in the installation path on Windows as well).
Thanks for looking into this breakage!

This change looks fine to me, but i'll leave it to someone more experienced with CMake to review this

There is also https://phabricator.kde.org/D23219 which implements a slightly different approach, but I don't know which is better either.

I wasn't aware of another revision being under review to fix the same issue, sorry.

D23219 is the simpler fix, using only the file name to create a target name. Unless this cmake module could be reused in other projects, that fix is sufficient to address this broken build issue.

If the cmake module is going to be reused, however, D23219 needs to be hardened further to address two potential breakages: if the file name has spaces in it, or if there are multiple files in the same repository that share the same name. We could guard against the first by using a STRING(REPLACE ...) fix like in this revision, and against the latter by checking with IF (TARGET ...) and adding an incrementing numeric suffix to the target name, as needed. The current patch can be reused in other projects as is.

If reuse is not a concern, I think I'd go with the simpler fix (D23219), to be honest.

karthikp abandoned this revision.Aug 20 2019, 5:30 AM

Abandoning this in favor of D23219, which is now hardened for reuse.