headers: incorrect relative path includes

[AdelKS]

Hello,

I installed the library so I can use it with other build systems (qmake and meson), and compilation fails due to incorrect relative path includes

• Headers do not include each other with proper relative paths.
• The generated qschematic_export.h header is at the root of the include directory, i.e in qschematic/qschematic_export.h, yet headers in subfolders include it as if it as the same level.

I had to do these changes to be able to compile so far

include path fixes

```patch diff --git a/qschematic/commands/commandbase.h b/qschematic/commands/commandbase.h index 5c5ecd6..d3db87a 100644 --- a/qschematic/commands/commandbase.h +++ b/qschematic/commands/commandbase.h @@ -1,6 +1,6 @@ #pragma once -#include "qschematic_export.h" +#include "../qschematic_export.h" #include diff --git a/qschematic/items/connector.h b/qschematic/items/connector.h index 672d1fc..1d8d4c7 100644 --- a/qschematic/items/connector.h +++ b/qschematic/items/connector.h @@ -1,8 +1,8 @@ #pragma once #include "item.h" -#include "wire_system/connectable.h" -#include "qschematic_export.h" +#include "../wire_system/connectable.h" +#include "../qschematic_export.h" namespace QSchematic { diff --git a/qschematic/items/item.h b/qschematic/items/item.h index ae419b8..5d736c3 100644 --- a/qschematic/items/item.h +++ b/qschematic/items/item.h @@ -6,7 +6,7 @@ #include "../types.h" #include "../settings.h" #include "itemfunctions.h" -#include "qschematic_export.h" +#include "../qschematic_export.h" #include #include @@ -14,6 +14,7 @@ namespace QSchematic { class Scene; + class Item; class QSCHEMATIC_EXPORT Item : public QGraphicsObject, diff --git a/qschematic/items/label.h b/qschematic/items/label.h index 76aacbe..2915826 100644 --- a/qschematic/items/label.h +++ b/qschematic/items/label.h @@ -3,7 +3,7 @@ #include #include "item.h" -#include "qschematic_export.h" +#include "../qschematic_export.h" namespace QSchematic { diff --git a/qschematic/items/node.h b/qschematic/items/node.h index 4a5bdcf..0c16ec6 100644 --- a/qschematic/items/node.h +++ b/qschematic/items/node.h @@ -4,7 +4,7 @@ #include "item.h" #include "connector.h" #include "../types.h" -#include "qschematic_export.h" +#include "../qschematic_export.h" class QGraphicsSceneMouseEvent; class QGraphicsSceneHoverEvent; diff --git a/qschematic/items/wire.h b/qschematic/items/wire.h index 6736797..99459a4 100644 --- a/qschematic/items/wire.h +++ b/qschematic/items/wire.h @@ -2,11 +2,11 @@ #include -#include "wire_system/point.h" -#include "wire_system/wire.h" +#include "../wire_system/point.h" +#include "../wire_system/wire.h" #include "item.h" #include "wirenet.h" -#include "qschematic_export.h" +#include "../qschematic_export.h" class QVector2D; diff --git a/qschematic/items/wirenet.h b/qschematic/items/wirenet.h index 28b2ea3..bbbcfac 100644 --- a/qschematic/items/wirenet.h +++ b/qschematic/items/wirenet.h @@ -5,9 +5,9 @@ #include #include -#include "wire_system/line.h" -#include "wire_system/net.h" -#include "qschematic_export.h" +#include "../wire_system/line.h" +#include "../wire_system/net.h" +#include "../qschematic_export.h" namespace wire_system { class point; diff --git a/qschematic/scene.h b/qschematic/scene.h index 1678678..20d4a1e 100644 --- a/qschematic/scene.h +++ b/qschematic/scene.h @@ -101,8 +101,8 @@ namespace QSchematic { signals: void modeChanged(int newMode); void isDirtyChanged(bool isDirty); - void itemAdded(std::shared_ptr item); - void itemRemoved(std::shared_ptr item); + void itemAdded(const std::shared_ptr item); + void itemRemoved(const std::shared_ptr item); void itemHighlighted(const std::shared_ptr& item); protected: diff --git a/qschematic/wire_system/connectable.h b/qschematic/wire_system/connectable.h index e1013e0..6c29ac7 100644 --- a/qschematic/wire_system/connectable.h +++ b/qschematic/wire_system/connectable.h @@ -1,6 +1,6 @@ #pragma once -#include "qschematic_export.h" +#include "../qschematic_export.h" namespace wire_system { @@ -9,4 +9,4 @@ namespace wire_system public: [[nodiscard]] virtual QPointF position() const = 0; }; -} \ No newline at end of file +} diff --git a/qschematic/wire_system/line.h b/qschematic/wire_system/line.h index 4aac288..dcfbc4f 100644 --- a/qschematic/wire_system/line.h +++ b/qschematic/wire_system/line.h @@ -1,6 +1,6 @@ #pragma once -#include "qschematic_export.h" +#include "../qschematic_export.h" #include diff --git a/qschematic/wire_system/manager.h b/qschematic/wire_system/manager.h index 87ceb7b..9ee7b8c 100644 --- a/qschematic/wire_system/manager.h +++ b/qschematic/wire_system/manager.h @@ -7,7 +7,7 @@ #include #include "../settings.h" -#include "qschematic_export.h" +#include "../qschematic_export.h" namespace QSchematic { diff --git a/qschematic/wire_system/net.h b/qschematic/wire_system/net.h index 5c2364f..b1b2a8a 100644 --- a/qschematic/wire_system/net.h +++ b/qschematic/wire_system/net.h @@ -1,6 +1,6 @@ #pragma once -#include "qschematic_export.h" +#include "../qschematic_export.h" #include #include diff --git a/qschematic/wire_system/point.h b/qschematic/wire_system/point.h index 2fb0b01..cfd2093 100644 --- a/qschematic/wire_system/point.h +++ b/qschematic/wire_system/point.h @@ -1,6 +1,6 @@ #pragma once -#include "qschematic_export.h" +#include "../qschematic_export.h" #include diff --git a/qschematic/wire_system/wire.h b/qschematic/wire_system/wire.h index 8ee90da..04b326c 100644 --- a/qschematic/wire_system/wire.h +++ b/qschematic/wire_system/wire.h @@ -5,7 +5,7 @@ #include #include "point.h" -#include "qschematic_export.h" +#include "../qschematic_export.h" class QVector2D; ```

But these changes break the building logic with cmake as it looks like it gives this generated header qschematic_export.h through an extra include directory directive.

Thanks!

Adel

[Tectu]

Hi,

Is this problem something you're encountering while building or while consuming after installation?

There is a line specifically adding the build directory as an include path to pull in qschematic_export.h during building: https://github.com/simulton/QSchematic/blob/de7ecb540d9cb75bca89f2f2dd7ad738e3ef60ab/qschematic/CMakeLists.txt#L107

During installation, qschematic_export.h is placed in the top-level include directory.

I'm not saying that there isn't something incorrect or no room for improvements but this library is currently being used successfully as-is in a various projects on Windows, MacOS, BSD and Linux so I'm keen to understand this problem.

[AdelKS]

I'm not saying that there isn't something incorrect or no room for improvements

And I think I did not chose my words properly. My apologies.

As you said, cmake handles this well. And I faced issues probably because qmake is not a good build system (that's why Qt moved away from it). And meson has good support for cmake so probably that would work well too (I can try and report back, but I won't be able for the next few weeks).

What I am suggesting in the end is that maybe it's better to take the simple road and to use relative imports instead of using the build system to add extra include directories. :thinking:

Is this problem something you're encountering while building or while consuming after installation?

It's while consuming after installation, here's an example project with Qt Creator and qmake that reproduces the issues I faced.

mainwindow.h

#pragma once

#include <QMainWindow>
#include <QWidget>
#include <qschematic/scene.h>
#include <qschematic/view.h>

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    MainWindow(QWidget *parent = nullptr)
        : QMainWindow(parent)
    {
         scene = new QSchematic::Scene;
         view = new QSchematic::View;

         view->setScene(scene);
         setCentralWidget(view);
    }

protected:
    QSchematic::Scene *scene;
    QSchematic::View *view;
};

main.cpp

#include <QApplication>

#include "mainwindow.h"

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    MainWindow w;
    w.show();
    return a.exec();
}

qschematic-test.pro

QT += core gui widgets

CONFIG += c++17

SOURCES += \
    main.cpp

HEADERS += \
    mainwindow.h

LIBS += -lqschematic

[Tectu]

And I think I did not chose my words properly. My apologies.

Thank you for elaborating. I understand the situation better now. I know the pain working with QMake. In fact, QSchematic was one of the first projects I transitioned over to CMake quite a while ago :D

I agree that this should be handled more properly. Some classes are even missing the import/export declarations. In some other libraries I've eventually removed this clutter as most of the classes are exported into a shared library anyway so we can just instruct the compiler to export all of them. What do you think?

[Tectu]

@AdelKS Could you give the feature/remove_exports branch a try?

[AdelKS]

Hello, I will be away from my computer for two weeks, I'll be back to you as soon as I can. Cheers.

[AdelKS]

The branch seem to fix the issues related to qschematic/qschematic_export.h but the issues with relative imports remain, for example:

/usr/include/qschematic/items/wire.h:5: error: wire_system/point.h: No such file or directory
In file included from /usr/include/qschematic/scene.h:12,
                 from ../qschematic-test/mainwindow.h:5,
                 from ../qschematic-test/main.cpp:1:
/usr/include/qschematic/items/wire.h:5:10: fatal error: wire_system/point.h: No such file or directory
    5 | #include "wire_system/point.h"
      |          ^~~~~~~~~~~~~~~~~~~~~

[AdelKS]

I believe the proper way to handle imports within the library is to use:

• relative paths: for the example above, to change #include "wire_system/point.h" to #include "../wire_system/point.h"
• "absolute" paths starting from the library's name: for the example above, to change #include "wire_system/point.h" to #include <qschematic/wire_system/point.h>

Tell me what you think !

Have a good weekend.

Adel

[Tectu]

Thank you for getting back to this!

I'm happy to hear that the export situation is resolved this way. I'll merge those changes shortly.

Regarding include paths: Ah yes, I fully agree. This definitely needs fixing. This was basically the first real-world C++ library I started many years ago without the experience present today. Mistakes were made. I am gladly going to clean those up.

[AdelKS]

Happy to help !

Haha I'm going through the same with my projects, where I see a lot of things I can do better now x)

[Tectu]

Well, we can only learn :)

Would you be able to test the feature/includes_cleanup branch?

[AdelKS]

Well, we can only learn :)

Indeed

Would you be able to test the feature/includes_cleanup branch?

looks like we are all good now!

[Tectu]

looks like we are all good now!

Awesome! Thanks for bringing this to my attention and testing it :)