Details

Reviewers

pino
asemke
sirgienko

Group Reviewers

Cantor

Commits

R55:53cdf514bf1b: Fix compilation issues on Windows

Summary

The linkage doesn't work currently on Windows because lib/test and backend/python libraries are using the same export flag than the lib/.

I have replaced the cantor_export.h flat file by calls to generate_export_header (via CMake) and put differentiated CANTOR_EXPORT/CANTOR_PYTHONBACKEND_EXPORT precompiler directives depending on the library being built.

Diff Detail

Repository

R55 Cantor

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

sdepiets created this revision.Oct 8 2018, 9:13 AM

Restricted Application added a project: KDE Edu. · View Herald TranscriptOct 8 2018, 9:13 AM

Restricted Application added a subscriber: kde-edu. · View Herald Transcript

sdepiets requested review of this revision.Oct 8 2018, 9:13 AM

Harbormaster completed remote builds in B3630: Diff 43113.Oct 8 2018, 9:13 AM

Do not copy the export header

Harbormaster completed remote builds in B3631: Diff 43114.Oct 8 2018, 9:16 AM

sdepiets edited the summary of this revision. (Show Details)Oct 8 2018, 9:34 AM

sdepiets added a reviewer: Cantor.

Thanks for this work.

src/backends/maxima/CMakeLists.txt
19–20	I think, you could remove this `if(NOT WIN32)`, becouse we don't use KPtyProcess with maxima. Also, need to remove preprocessor's statement in `maximasession.h:31`

please remove all the <repository.h> changes from this patch, since they are nothing to do with the actual export fixes

src/backends/python/pythonbackend.h
25	this ought to be <...>, because it's a generated header in a directory different than the same source directory of this file
src/backends/python/pythonsession.h
26	ditto
src/lib/test/backendtest.h
28	ditto

This revision now requires changes to proceed.Oct 8 2018, 5:48 PM

Requested changes

Harbormaster completed remote builds in B3662: Diff 43179.Oct 9 2018, 1:05 AM

sdepiets marked 3 inline comments as done.Oct 9 2018, 1:06 AM

please amend the commit message to not mention the <repository.h> changes

src/backends/maxima/maximasession.h
31–36 ↗	(On Diff #43179)	while this change is OK, it is unrelated to this patch

This revision now requires changes to proceed.Oct 9 2018, 4:57 AM

sdepiets edited the summary of this revision. (Show Details)Oct 9 2018, 5:08 AM

sdepiets added inline comments.Oct 9 2018, 3:54 PM

src/backends/maxima/maximasession.h
31–36 ↗	(On Diff #43179)	Do you want me to change this in a separate commit ?

@sdepiets for the sake of completeness, can you please tell what the issue were with compiling (or rather linking?) on windows?

Furthermore, GenerateExportHeader seems to be depricated:
https://cmake.org/cmake/help/v3.0/module/GenerateExportHeader.html

sirgienko added inline comments.Oct 9 2018, 4:46 PM

src/backends/python/pythonbackend.h
27	Is this export necesary? Other backends don't hasen't used `cantor_export.h`, so maybe this backend shouldn't do it too?

In D16039#340061, @asemke wrote:

@sdepiets for the sake of completeness, can you please tell what the issue were with compiling (or rather linking?) on windows?

The problem was with lib/test and python/backend libraries sharing the same export flag than lib.

Furthermore, GenerateExportHeader seems to be depricated:
https://cmake.org/cmake/help/v3.0/module/GenerateExportHeader.html

That's just for ADD_COMPILER_EXPORT_FLAGS no ?

In D16039#340314, @sdepiets wrote:

In D16039#340061, @asemke wrote:

@sdepiets for the sake of completeness, can you please tell what the issue were with compiling (or rather linking?) on windows?

The problem was with lib/test and python/backend libraries sharing the same export flag than lib.

This is a good thing to mention in the commit message, instead of a generic "a couple of issues".

src/backends/maxima/maximasession.h
31–36 ↗	(On Diff #43179)	Yes, which does not need a review (just commit it directly) IMHO.
src/backends/python/pythonbackend.h
27	Other backends do not have a shared library, like cantor_pythonbackend. Since both cantor_python2backend and cantor_python3backend are implemented using cantor_pythonbackend, then its symbols are needed.

Unused forward declarations will be removed separately

Harbormaster completed remote builds in B3705: Diff 43264.Oct 10 2018, 6:15 AM

sdepiets edited the summary of this revision. (Show Details)Oct 10 2018, 6:17 AM

Seems OK for me -- wait a bit in case the other reviewers have concerns.

This revision is now accepted and ready to land.Oct 10 2018, 6:24 AM

In D16039#340314, @sdepiets wrote:

In D16039#340061, @asemke wrote:

@sdepiets for the sake of completeness, can you please tell what the issue were with compiling (or rather linking?) on windows?

The problem was with lib/test and python/backend libraries sharing the same export flag than lib.

Ok.

Furthermore, GenerateExportHeader seems to be depricated:
https://cmake.org/cmake/help/v3.0/module/GenerateExportHeader.html

That's just for ADD_COMPILER_EXPORT_FLAGS no ?

You're right. Thanks.

sirgienko accepted this revision.Oct 10 2018, 7:06 AM

Closed by commit R55:53cdf514bf1b: Fix compilation issues on Windows (authored by sdepiets). · Explain WhyOct 10 2018, 2:13 PM

This revision was automatically updated to reflect the committed changes.

		Path
M		CMakeLists.txt (3 lines)
M		src/backends/maxima/CMakeLists.txt (4 lines)
M		src/backends/python/CMakeLists.txt (2 lines)
M		src/backends/python/pythonbackend.h (3 lines)
M		src/backends/python/pythonsession.h (3 lines)
M		src/lib/CMakeLists.txt (2 lines)
M		src/lib/assistant.h (2 lines)
M		src/lib/backend.h (2 lines)
D	M	src/lib/cantor_export.h (48 lines)
M		src/lib/completionobject.h (2 lines)
M		src/lib/defaulthighlighter.h (2 lines)
M		src/lib/directives/plotdirectives.h (2 lines)
M		src/lib/epsresult.h (2 lines)
M		src/lib/expression.h (2 lines)
M		src/lib/extension.h (2 lines)
M		src/lib/latexrenderer.h (2 lines)
M		src/lib/latexresult.h (2 lines)
M		src/lib/panelplugin.h (2 lines)
M		src/lib/panelpluginhandler.h (2 lines)
M		src/lib/result.h (2 lines)
M		src/lib/session.h (2 lines)
M		src/lib/syntaxhelpobject.h (2 lines)
M		src/lib/test/CMakeLists.txt (1 line)
M		src/lib/test/backendtest.h (2 lines)
M		src/lib/textresult.h (2 lines)
M		src/lib/worksheetaccess.h (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	43113	8367cda		Oct 8 2018, 9:13 AM	★	★
Diff 2	43114	8367cda	Do not copy the export header	Oct 8 2018, 9:16 AM	★	★
Diff 3	43179	8367cda	Requested changes	Oct 9 2018, 1:05 AM	★	★
Diff 4	43264	8367cda	Unused forward declarations will be removed separately	Oct 10 2018, 6:15 AM	★	★
Diff 5	43309	b9b5193	R55:53cdf514bf1ba85ed5e6b3de04807edce2a06b00	Oct 10 2018, 2:13 PM	★	★

Fix compilation issues on Windows
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 43309

CMakeLists.txt

src/backends/maxima/CMakeLists.txt

src/backends/python/CMakeLists.txt

src/backends/python/pythonbackend.h

src/backends/python/pythonsession.h

src/lib/CMakeLists.txt

src/lib/assistant.h

src/lib/backend.h

src/lib/cantor_export.h

src/lib/completionobject.h

src/lib/defaulthighlighter.h

src/lib/directives/plotdirectives.h

src/lib/epsresult.h

src/lib/expression.h

src/lib/extension.h

src/lib/latexrenderer.h

src/lib/latexresult.h

src/lib/panelplugin.h

src/lib/panelpluginhandler.h

src/lib/result.h

src/lib/session.h

src/lib/syntaxhelpobject.h

src/lib/test/CMakeLists.txt

src/lib/test/backendtest.h

src/lib/textresult.h

src/lib/worksheetaccess.h

Fix compilation issues on WindowsClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 43309

CMakeLists.txt

src/backends/maxima/CMakeLists.txt

src/backends/python/CMakeLists.txt

src/backends/python/pythonbackend.h

src/backends/python/pythonsession.h

src/lib/CMakeLists.txt

src/lib/assistant.h

src/lib/backend.h

src/lib/cantor_export.h

src/lib/completionobject.h

src/lib/defaulthighlighter.h

src/lib/directives/plotdirectives.h

src/lib/epsresult.h

src/lib/expression.h

src/lib/extension.h

src/lib/latexrenderer.h

src/lib/latexresult.h

src/lib/panelplugin.h

src/lib/panelpluginhandler.h

src/lib/result.h

src/lib/session.h

src/lib/syntaxhelpobject.h

src/lib/test/CMakeLists.txt

src/lib/test/backendtest.h

src/lib/textresult.h

src/lib/worksheetaccess.h

Fix compilation issues on Windows
ClosedPublic
Actions

Revision Contents
Changeset List