Remove export header from static install
ClosedPublic

Authored by masonm on Jan 28 2020, 12:22 AM.

Details

Summary

When building the "install" target with a static Kirigami build an error occurs due to the missing file kirigami2_export.h.

Test Plan

Download Kirigami sources, create build directory, create install directory, and then something similar to:

INSTALL_DIR=$HOME/tmp-install
QT_INSTALL_DIR=$HOME/qt-5.12.6
ECM_DIR=$HOME/tmp-install  # Assume ECM was also "make installed" into this dir
KIRIGAMI_SRC=$HOME/kirigami

cmake -DBUILD_SHARED_LIBS=OFF -DCMAKE_INSTALL_PREFIX=$INSTALL_DIR -DCMAKE_PREFIX_PATH=$QT_INSTALL_DIR;$ECM_DIR $KIRIGAMI_SRC
cmake --build . --target install

Before the patch this will fail because of missing kirigami2_export.h file.

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
masonm created this revision.Jan 28 2020, 12:22 AM
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptJan 28 2020, 12:22 AM
masonm requested review of this revision.Jan 28 2020, 12:22 AM
ndavis edited reviewers, added: Kirigami, mart; removed: VDG.Jan 28 2020, 1:29 AM
apol accepted this revision.Jan 28 2020, 1:39 AM
This revision is now accepted and ready to land.Jan 28 2020, 1:39 AM
apol requested changes to this revision.Jan 28 2020, 1:40 AM

Wait no, are you sure? kirigami headers will still include the file...

This revision now requires changes to proceed.Jan 28 2020, 1:40 AM

Ah, I'll take a closer look.

It appears all the sources which reference the export header have an appropriate guard on them:

#ifndef KIRIGAMI_BUILD_TYPE_STATIC
#include <kirigami2_export.h>
#endif

I found these in tabletmodewatcher.h, kirigamipluginfactory.h, platformtheme.h

masonm requested review of this revision.Mar 18 2020, 9:11 PM

Hello,

Just following up on this change review—it all looks good to me. Can we merge it in?

Thanks!

mart accepted this revision.Mar 19 2020, 5:00 PM

@apol is your question answered now / can I land this?

This revision was not accepted when it landed; it landed in state Needs Review.Mar 22 2020, 1:59 AM
This revision was automatically updated to reflect the committed changes.