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

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 3662
Build 3680: arc lint + arc unit

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	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	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	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/maxima/maximasession.h (6 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 43179

CMakeLists.txt

src/backends/maxima/CMakeLists.txt

src/backends/maxima/maximasession.h

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 43179

CMakeLists.txt

src/backends/maxima/CMakeLists.txt

src/backends/maxima/maximasession.h

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