Hi, Michael!

Isn't it done automatically by the compiler? I guess it worked without explicit definitions for quite a long time. Are you sure they are needed?

Or it is somehow related to Visual Studio 2013 only?

Oh, and I see you did this only for stuff that Krita uses (i.e. built for the productset KRITA).
Are you able to extend this patch for all of Calligra? If not, I would do a complementary fix commit then afterwards, but perhaps you can do a complete fix already :)

Dmitry: Yes, this seems to be one of those mysterious kde4 paradigms that we don't have to worry about anymore. (Can you tell I'm much more enthusiastic than Boudewijn about the upgrade to Frameworks 5?)

Kossebau: Thank you for the input. Switching to default Cmake methods sounds like the right way to go. Unfortunately I'm not familiar with building any of Calligra except Krita, and the only way to test these macros are working properly is to build on Windows. What I could do would be submit a patch that I know works on Krita and then attempt to make the same changes for the rest of Calligra by analogy, but I'd need someone else to check the rest of the suite for me. I could also add comments everywhere I haven't verified.

In D183#3292, @abrahams wrote:

Kossebau: Thank you for the input. Switching to default Cmake methods sounds like the right way to go. Unfortunately I'm not familiar with building any of Calligra except Krita, and the only way to test these macros are working properly is to build on Windows. What I could do would be submit a patch that I know works on Krita and then attempt to make the same changes for the rest of Calligra by analogy, but I'd need someone else to check the rest of the suite for me. I could also add comments everywhere I haven't verified.

That would be great, please do. BTW, just looking into turning the pigment_export.h header into an autogenerated one, not sure yet when this will land, but if pigment_export.h no longer exists in the sources, do not be surprised :)

Sorry for the delay. I wanted to install Windows 10 before doing anything else more on Windows. However there are still a few machete chops before I make it back to where I was.

Sorry this is long and messy. Most of the files are being updated with a simple find and replace here to replace the old unified krita_export.h to the library-appropriate kritaimage_export.h, kritaui_export.h, etc.

The interesting stuff is in the cmake lists.

Lots of work :) But, oh dear, I fear we misunderstood each other a little?

My proposal was to not explicitely set the property DEFINE_SYMBOL at all, but simply rely on the default as set by cmake, especially as the generate_export_header from cmake could be assumed to play well with the default, so less code for us to maintain and care for.
And also proposed that to get to a minimal change with this patch as needed to get things building on windows (no idea why current code still works on linux, not investigated).
But now you went and added the property setting explicitely even everywhere? :) Does the default not work for you, or do you think explicit is being better?

Additionally you also
a) turned more export headers to generated one
b) "fixed" krita_export.h to better named kritaimage_export.h
Things I personally welcome :)
Nitpick: Ideally would have been done as separate reviews, to ease your reviewer's life (and also allow to get the parts which are fine in already, while the rest is under discussion).

For b) I think this better is done in the calligra/2.9 branch perhaps first, as it touches lots of files and thus has a higher risk to result in merge/cherry-pick conflicts? Hard to tell for me, left for boud/dimitry to comment on.

So my only nitpick would be: why all the explicit symbol property setting?
Have not tested the build yet, so cannot give feedback on that.

In D183#4249, @rempt wrote:

If this builds with msvc and gcc, I'm fine with it... I'm not sure what is broken, but that might be frameworks stuff?

Oh well, I should not beg people to comment on things when they are too tired to take a closer look ;)

So some additional comments:

For one Boud would not disagree on doing the "krita_export.h" rename to "kritaimage_export.h" first in 2.9:
[18:19] <frinring> boud: (am a little behind due to other things, so may not have read all emails) I see a little chance for "hours fixing things" because of the change of the include dir to all those many headers. that's why I ask if you see the chance as well, or not, because you might be more able to guess how big chances for conflicts are
[18:20] <frinring> instincitely I would propose to rename krita_export.h in 2.9 for reduced chances of conflicts. but well, I am not the one who really works on krita code and will have to port/merge patches
[18:21] <boud> yes... doing that in 2.9 makes sense

For the other seems he has no opinion on explicitely setting the property DEFINE_SYMBOL :)
but I do, which is, NOT to do it explicitely (unless it is really needed). So I would like to ask to change this patch to that, relying on the cmake defaults, like done with so many other things.

I can help with doing the rename of "krita_export.h" to "kritaimage_export.h" in 2.9 branch today or tomorrow to speed this up, so on the next merge to frameworks to happy possibly this Friday that one would be done already. Just tell, ready to do that in the background.

Hi Friedrich: thanks for the help. Again since CMake documentation is so sparse most of this task was mindless replacing for me to get something that worked. It is easy to simply get rid of the DEFINE_SYMBOLS as long as you are sure they are redundant.

Things are not as simple as renaming krita_export.h to kritaimage_export.h. The former was actually a collection of different symbol definitions. The intermediate change you're proposing would mean you will need to create the following new files: (don't quote me on the names) kritaimage_export.h, kritaui_export.h, kritasketch_export.h, paintop_export.h, kritabrush_export.h. The subsequent commit would then delete these files. It seems like a lot of work for a minor bit of history sanitizing.

When I fix this again I will remove any changes that touch Kexi.

I've ported quite a few libs to Qt5 and cmake, DEFINE_SYMBOL is not needed at all.

If this is a way to avoid merge conflicts... well this becoming complex workaround on its own :) If this was for 2.9 I would't care.

In 3.x there shouldn't exist any *_export.h files if you ask me.

In D183#4272, @abrahams wrote:

Things are not as simple as renaming krita_export.h to kritaimage_export.h. The former was actually a collection of different symbol definitions. The intermediate change you're proposing would mean you will need to create the following new files: (don't quote me on the names) kritaimage_export.h, kritaui_export.h, kritasketch_export.h, paintop_export.h, kritabrush_export.h. The subsequent commit would then delete these files. It seems like a lot of work for a minor bit of history sanitizing.

Well, the include lines have to be changed anyway (but right, this patch has the work done already). Adding a few export headers temporarily seems not such a problem to me.

This is not about history sanitizing. It is about getting a smaller diff between 2.9 and frameworks. Remember that some people are still working on features in Krita in the 2.9 branch (or branches based on that). And when their commits are merged to frameworks (or cherry-picked in the future), these different include lines have chances to result in merge conflicts, which can be painful to resolve.
Oh well. Not being a Krita dev doing development in 2.9, I perhaps should care less about that :) I hinted to this possible problem, so I am out now on this point :)

Yes, with the recent change that became the ultimate goal: delete the xxx_exports.h files. I know the patch is big and messy, but you can see it includes about half a dozen such deletions. That's the reason why splitting it into two parts is awkward.

I'll go ahead trying to do without the DEFINE_SYMBOLS. If you would like this to become a slightly bigger, messier patch, I can hunt for and expunge the remaining xxx_exports.h files as well.

In D183#4274, @staniek wrote:

In 3.x there shouldn't exist any *_export.h files if you ask me.

Yes, multiple persons already changed a few files to be generated, so that seems an agreed goal :) The initial patch just was only about a single export header and only grew later to cover more, so that's why there was more disucussion.

Just, some of the existing export headers define macros for symbols that should be only exported for test builds. Which might need some more thinking, unless there is an easy solution?

In D183#4276, @abrahams wrote:

I'll go ahead trying to do without the DEFINE_SYMBOLS. If you would like this to become a slightly bigger, messier patch, I can hunt for and expunge the remaining xxx_exports.h files as well.

Please let's keep commits as less messy as possible :) No need to get rid of all export headers in one go.

OK, I understand now. The problem with doing it personally is that maintaining separate 2.9 and 3.0 builds on Windows is a huge challenge. (Each c:\kde_XXX\ folder is over 20GB. That's one for debug and one for release, per version of Qt.) Since installing Windows 10 I have not bothered with 2.9 anymore. It's not simply adding the export headers, but the many hours of setting up a new build environment and testing to make sure it's all working properly that makes me averse to doing the split myself.

In D183#4278, @abrahams wrote:

OK, I understand now. The problem with doing it personally is that maintaining separate 2.9 and 3.0 builds on Windows is a huge challenge, and since installing Windows 10 I have not bothered with 2.9 anymore. It's not simply adding the export headers, but the many hours of setting up a new build environment and testing to make sure it's all working properly that makes me averse to doing the split myself.

I see. So given I insisted on this so much, would you be okay with me doing the according change to 2.9 (by taking the diff to all the krita files and creating the respecitve expoort headers), so I would back the noise I made myself? :) Would do tonight. and should arrive in frameworks on Friday. Yes, means you mght need to adapt your patch once more (hopefully though the includes lines would resolve without issues), but this might still in the end be easier because the affected things are known and thus easier to solve.

In D183#4279, @kossebau wrote:

In D183#4278, @abrahams wrote:

OK, I understand now. The problem with doing it personally is that maintaining separate 2.9 and 3.0 builds on Windows is a huge challenge, and since installing Windows 10 I have not bothered with 2.9 anymore. It's not simply adding the export headers, but the many hours of setting up a new build environment and testing to make sure it's all working properly that makes me averse to doing the split myself.

I see. So given I insisted on this so much, would you be okay with me doing the according change to 2.9 (by taking the diff to all the krita files and creating the respecitve expoort headers), so I would back the noise I made myself? :) Would do tonight. and should arrive in frameworks on Friday. Yes, means you mght need to adapt your patch once more (hopefully though the includes lines would resolve without issues), but this might still in the end be easier because the affected things are known and thus easier to solve.

Absolutely, I have no problem rebasing the patch against Frameworks updates. I just want to avoid messing around with 2.9.

Like skipping the changes for kexi/, I propose to split the final patch: skip the proper changes in krita/ and in whatever dirs keeps krita touch for calligra/2.9 (pigment, some libs/?). And then commit the non-kexi/non-krita things.

Regarding the exporting extra symbols for testing. I see that builds with testing enabled are not for general distribution. They contain mock code, at least for Kexi they can do. They contain double checking that's also not needed/efficient. Then may depend on the Debug build type. So for me it's perfectly OK to export them in these builds.

How to do that?

e.g. in MYLIB lib's source dir:

if(BUILD_TESTING)
  add_definitions(-DMYLIB_TEST_EXPORT=MYLIB_EXPORT)
else()
  add_definitions(-DMYLIB_TEST_EXPORT=)
endif()

Then use MYLIB_TEST_EXPORT to export the extra symbols.
It's not a lot of code, and well... not every lib adds extra exports.

In D183#4283, @staniek wrote:

Like skipping the changes for kexi/, I propose to split the final patch: skip the proper changes in krita/ and in whatever dirs keeps krita touch for calligra/2.9 (pigment, some libs/?). And then commit the non-kexi/non-krita things.

Regarding the exporting extra symbols for testing. I see that builds with testing enabled are not for general distribution. They contain mock code, at least for Kexi they can do. They contain double checking that's also not needed/efficient. Then may depend on the Debug build type. So for me it's perfectly OK to export them in these builds.

How to do that?

e.g. in MYLIB lib's source dir:

if(BUILD_TESTING)
  add_definitions(-DMYLIB_TEST_EXPORT=MYLIB_EXPORT)
else()
  add_definitions(-DMYLIB_TEST_EXPORT=)
endif()

Then use MYLIB_TEST_EXPORT to export the extra symbols.
It's not a lot of code, and well... not every lib adds extra exports.

Does this need to happen for every library? Should it be automated as a CMake macro? Should it be added in this patch or a different one? I'm thinking this discussion should be moved to Maniphest, because the various requirements are becoming confusing since they can't be split into subtasks.

Yes, a simple macro CalligraAddTestExportMacro.cmake having calligra_add_test_export_macro(libbasename) macro could be added to calligra/cmake/modules/. Feel free to add a task for developing it together with the rest of the patch.

No problem. Is this good to go?