simulton / QSchematic

A library that allows creating diagrams such as flowcharts or even proper engineering schematics within a Qt application.
https://simulton.com
MIT License
231 stars 60 forks source link

Help for building the library #63

Closed TobiasWallner closed 6 months ago

TobiasWallner commented 7 months ago

Hi, Could someone please help me, it seems like I have some troubles building this library.


I am working on a windows 10 machine. I first tried building with Visual Studio:

cd qschematic
cmake -B build -G "Visual Studio 17 2022" -D Qt6_DIR="C:\Qt\6.6.1\mingw_64\lib\cmake\Qt6"
cmake --build build --config Release
cd ..

which resulted in the following error:

MSBuild version 17.5.1+f6fdcf537 for .NET Framework
MSBUILD : error MSB1009: Project file does not exist.
Switch: ALL_BUILD.vcxproj

And then I tried it wich clang and ninja

cd qschematic
SET CC=clang
SET CXX=clang++
cmake -B build -G "Ninja Multi-Config" -D Qt6_DIR="C:\Qt\6.6.1\mingw_64\lib\cmake\Qt6"
cmake --build build --config Release
cd ..

which resulted in the following error:

ninja: error: CMakeFiles/impl-Release.ninja:737: multiple rules generate qschematic/Release/qschematic.lib

Tanks for any help in advance.

P.S.: Your library looks really cool. I would like to use it to build an open source graphical tool that autogenerates hardware description language code like VHDL and Verilog for ASIC and FPGA developement.

Tectu commented 7 months ago

Hey,

Hmm... I thought we fixed that problem in #50. Is this the same scenario? Are you using QSchematic 1.5.1 or newer?

That is a Windows-only configuration problem. Basically, the shared and the static library both have the same name. On Windows, this results in qschematic.lib being generated twice whereas on every (?) other platform it just results in having libqschematic.a and libqschematic.so.

This was addressed by making QSCHEMATIC_BUILD_STATIC and QSCHEMATIC_BUILD_SHARED mutually exclusive when using MSVC. But in your second case you're not using MSVC so I wonder whether we should generally change that restriction check from checking for MSVC to checking for Windows. Could you try building again but only setting one of those variable to ON manually?

Regarding your first building attempt: I have no idea. I usually just open the VisualStudio application and open the CMake project in there. Worked with QSchematic the last time I tried.

P.S.: Your library looks really cool. I would like to use it to build an open source graphical tool that autogenerates hardware description language code like VHDL and Verilog for ASIC and FPGA developement.

Nice! That is exactly the use case which made me start this library back in 2015 :)

TobiasWallner commented 7 months ago

Are you using QSchematic 1.5.1 or newer?

Yes I am using 1.5.1 QSchematic and 1.8.1 GPDS

Ok so it seems it may actually be a GPDS related issue.

building with;

cmake -B build -G "Ninja Multi-Config" -D Qt6_DIR="C:\Qt\6.6.1\mingw_64\lib\cmake\Qt6" -D QSCHEMATIC_BUILD_STATIC=ON -D QSCHEMATIC_BUILD_SHARED=OFF -D QSCHEMATIC_BUILD_DEMO=OFF -D QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD=ON 

Seems to work until: cmake --build build, then the error:

ninja: error: CMakeFiles/impl-Debug.ninja:587: multiple rules generate _deps/gpds-build/lib/Debug/gpdsd.lib

appears.

I tried changing my building strategy and install GPDS first with:

cmake -B build -S .
cmake --build build --config Release
cmake --install build

which worked just fine. And only the static version was bulit

Installing then QSchematic with QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD=OFF

resulted in the following:

>cmake -B build -G "Ninja Multi-Config" -D Qt6_DIR="C:\Qt\6.6.1\mingw_64\lib\cmake\Qt6" -D QSCHEMATIC_BUILD_STATIC=ON -D QSCHEMATIC_BUILD_SHARED=OFF -D QSCHEMATIC_BUILD_DEMO=OFF -D QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD=OFF
CMake Error at C:/Program Files/CMake/share/cmake-3.26/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)
Call Stack (most recent call first):
  C:/Program Files/CMake/share/cmake-3.26/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  C:/Program Files/CMake/share/cmake-3.26/Modules/FindPkgConfig.cmake:99 (find_package_handle_standard_args)
  C:/Program Files/CMake/share/cmake-3.26/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
  C:/Program Files (x86)/gpds/lib/cmake/gpds/gpds-config.cmake:9 (find_dependency)
  qschematic/external.cmake:25 (find_package)
  qschematic/CMakeLists.txt:2 (include)

-- Configuring incomplete, errors occurred!

Installing only with MSVC with QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD=ON yields the following error:

CMake Error at qschematic/external.cmake:22 (add_library):
  add_library cannot create ALIAS target "gpds::gpds-shared" because target
  "gpds-shared" does not already exist.
Call Stack (most recent call first):
  qschematic/CMakeLists.txt:2 (include)
Tectu commented 7 months ago

and 1.8.1 GPDS Ok so it seems it may actually be a GPDS related issue.

The same issue occurred over at GPDS and was already fixed in 1.8.0. The gist of it is that whenever we're building under a native Windows environment (no matter the compiler), X_BUILD_STATIC and X_BUILD_SHARED must be mutually exclusive (where X is QSCHEMATIC and GPDS).

resulted in the following:

Interesting. That issue you should not be encountering if you were truly using GPDS 1.8.1 instead of the current GPDS master branch.

Installing only with MSVC with QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD=ON yields the following error:

That is an open issue I don't know how to address properly yet. I have to check whether those alias targets should be provided by the consuming library or the providing library in CMake. See #53 for a temporary workaround.


Could you try using the GPDS 1.8.1 release as well as the workaround shown in #53?

Tectu commented 7 months ago

Alright so the error you got here:

>cmake -B build -G "Ninja Multi-Config" -D Qt6_DIR="C:\Qt\6.6.1\mingw_64\lib\cmake\Qt6" -D QSCHEMATIC_BUILD_STATIC=ON -D QSCHEMATIC_BUILD_SHARED=OFF -D QSCHEMATIC_BUILD_DEMO=OFF -D QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD=OFF
CMake Error at C:/Program Files/CMake/share/cmake-3.26/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)

That was a bug in gpds-config.cmake.in which was just fixed: https://github.com/simulton/gpds/commit/0634f21bcc8e5aa9f98ca0ed57499c4d9c2521dc A subsequent patch release 1.8.3 has just shipped.

bjornstromberg commented 7 months ago

a note on Qt6 and microsoft toolchain, I'm using only unix based toolchains so nothing that will affect me, but for whoever that might bite..

Qt-dev ML: looks like they will bump ms-toolchain from msvc2019 to msvc2022 in Qt6.8 from what it seems.

edit: reason for posting this here, is i saw @TobiasWallner using ms vs 17 (2022) which will be the minimum supported as i understand from Qt6.8 so its probably interesting information.

TobiasWallner commented 6 months ago

Sry coming back to you that late. Was a challenging week for me.

So, the workaround from #53 seems to work and also 1.8.3 of gpds seems to work.

Everything seems to compile but there are some linking errors now, ... and their kinda different depending on the toolchain I use.


So with GCC 13.2.0 I get a lot of this:

FAILED: qschematic/wire_system/test/Release/qschematic-wiresystem-tests.exe
cmd.exe /C "cd . && C:\msys64\ucrt64\bin\g++.exe -O3 -DNDEBUG  qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/test_main.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/line.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/manager.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/net.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/point.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/wire.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/__/utils.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/__/settings.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/tests/manager.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/tests/nets.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/tests/wire.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/tests/line.cpp.obj -o qschematic\wire_system\test\Release\qschematic-wiresystem-tests.exe -Wl,--out-implib,qschematic\wire_system\test\Release\libqschematic-wiresystem-tests.dll.a -Wl,--major-image-version,0,--minor-image-version,0  qschematic/Release/libqschematic.a  C:/Qt/6.6.1/mingw_64/lib/libQt6Widgets.a  C:/Qt/6.6.1/mingw_64/lib/libQt6Gui.a  C:/Qt/6.6.1/mingw_64/lib/libQt6Core.a  -lmpr  -luserenv  -ld3d11  -ldxgi  -ldxguid  -ld3d12  "C:/Program Files (x86)/gpds/lib/gpds.lib"  -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 && cd ."
C:/msys64/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: qschematic/Release/libqschematic.a(connector.cpp.obj):connector.cpp:(.text+0x69c): undefined reference to `gpds::value::~value()'

Thats, not the only error, but I do not want to paste the whole error code. It just cannot find anything from gpds::value::*. But gpts is installed and you can see that is useing this "C:/Program Files (x86)/gpds/lib/gpds.lib" for linking.


and with clang 16.0.3 I get the following:

[47/47] Linking CXX executable qschematic\wire_system\test\Release\qschematic-wiresystem-tests.exe
FAILED: qschematic/wire_system/test/Release/qschematic-wiresystem-tests.exe
cmd.exe /C "cd . && C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE -fuse-ld=lld-link -nostartfiles -nostdlib -O3 -DNDEBUG -D_DLL -D_MT -Xclang --dependent-lib=msvcrt -Xlinker /subsystem:console qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/test_main.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/line.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/manager.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/net.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/point.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/wire.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/__/utils.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/__/settings.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/tests/manager.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/tests/nets.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/tests/wire.cpp.obj qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/tests/line.cpp.obj -o qschematic\wire_system\test\Release\qschematic-wiresystem-tests.exe -Xlinker /MANIFEST:EMBED -Xlinker /implib:qschematic\wire_system\test\Release\qschematic-wiresystem-tests.lib -Xlinker /pdb:qschematic\wire_system\test\Release\qschematic-wiresystem-tests.pdb -Xlinker /version:0.0   qschematic/Release/qschematic.lib  C:/Qt/6.6.1/mingw_64/lib/libQt6Widgets.a  C:/Qt/6.6.1/mingw_64/lib/libQt6Gui.a  C:/Qt/6.6.1/mingw_64/lib/libQt6Core.a  -lmpr.lib  -luserenv.lib  -ld3d11.lib  -ldxgi.lib  -ldxguid.lib  -ld3d12.lib  "C:/Program Files (x86)/gpds/lib/gpds.lib"  -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 -loldnames  && cd ."
lld-link: error: undefined symbol: __declspec(dllimport) public: __cdecl QLineF::QLineF(class QPointF const &, class QPointF const &)
>>> referenced by qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/line.cpp.obj:(public: double __cdecl wire_system::line::length(void) const)
>>> referenced by qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/line.cpp.obj:(public: bool __cdecl wire_system::line::contains_point(class QPointF const &, double) const)
>>> referenced by qschematic/wire_system/test/CMakeFiles/qschematic-wiresystem-tests.dir/Release/__/line.cpp.obj:(public: class QLineF __cdecl wire_system::line::toLineF(void) const)
>>> referenced 18 more times

I was compiling with:

cmake -B build -G "Ninja Multi-Config" -D Qt6_DIR="C:\Qt\6.6.1\mingw_64\lib\cmake\Qt6" -D QSCHEMATIC_BUILD_STATIC=ON -D QSCHEMATIC_BUILD_SHARED=OFF -D QSCHEMATIC_BUILD_DEMO=OFF -D QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD=OFF -D GPDS_BUILD_STATIC=ON -D GPDS_BUILD_SHARED=OFF
cmake --build build --config Release
bjornstromberg commented 6 months ago

@TobiasWallner to start with im not going to dig deep into windows stuff... but more general stuff should still apply..

I was compiling with:

cmake -B build -G "Ninja Multi-Config" -D Qt6_DIR="C:\Qt\6.6.1\mingw_64\lib\cmake\Qt6" -D QSCHEMATIC_BUILD_STATIC=ON -D QSCHEMATIC_BUILD_SHARED=OFF -D QSCHEMATIC_BUILD_DEMO=OFF -D QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD=OFF -D GPDS_BUILD_STATIC=ON -D GPDS_BUILD_SHARED=OFF
cmake --build build --config Release

the first thing i see is that you seem to build GPDS and QSchematic at the same time? probably as bundled software driven from a cmake superbuild that runs stuff in correct order. if i remember correctly this was not supported due event order, cmake expects the modules it later will require to find to be configured, built and installed in sequential order, not to configure all, then build all, then install all that a non-complex superbuild does.

the way i package them on linux, and install as packages makes the run first to build and install the output into a staging area, this way next layer can find the files correctly via the cmake package configuration files and the targets that generates during the 'install' stage. in your case i would setup a dedicated staging dir and a powershell script to execute stuff in correct order. or just manually configure, build, install the gpds first, then same process with QSchematic, by now everything has happened in correct order.

so in pseudo shell script

for library in gpds qschematic qschematic-derived-app-or-library
do
  DESTDIR=<staging-prefix> cmake -B <builddir> -S <srcdir> -G [...] -D [...] 
  cmake --build <builddir>
  cmake --install <builddir>
done

have a look at https://github.com/bjornstromberg/alpine-extra-repos/tree/main/simulton and you have the process in the APKBUILD scripts modify as approprite for windows environment.

@Tectu to fix support for superbuild cmake layout just adding add_library({gpds,qschematic}::${target} ALIAS ${target}) in function(setup_target_common target) should do the trick.. both qschematic and gpds has the same issue.

TobiasWallner commented 6 months ago

@bjornstromberg First of all, you seem like a way more advanced programmer than I am. I am very thankfull that you took the time to help me.

or just manually configure, build, install the gpds first, then same process with QSchematic

Yes that worked. Sry for the bother.

TobiasWallner commented 6 months ago

Thanks for the time and the help

bjornstromberg commented 6 months ago

@bjornstromberg First of all, you seem like a way more advanced programmer than I am. I am very thankfull that you took the time to help me.

been doing software in c++/qt for more or less the last decade, so have encountered a few odd things with cmake and qt..

made my own share of mistakes too..

or just manually configure, build, install the gpds first, then same process with QSchematic

Yes that worked. Sry for the bother.

the best tip i have, is collect a knowledge base on every error you do, you dont have to keep it public, but keep a little black book for yourself because in 6 months, 2 years or a decade, you will know that you been 'bitten' by some error again, but likely wont know where to find the information to solve it. you will have to collect a treasure of the errors you do..

this is the tip i wish i got way back when i started out, cause i started compiling my little black book a few years back after getting the tip on some random blog i dont even remember today..

knowning what does not work, is more valuable then the stuff that you know works.. as the black book gets bigger and harder to find info in, you will digitalize it and continue to evolve it to your special needs, it will become like a smart assistant..

and good luck on your journey @TobiasWallner !

Tectu commented 6 months ago

@Tectu to fix support for superbuild cmake layout just adding add_library({gpds,qschematic}::${target} ALIAS ${target}) in function(setup_target_common target) should do the trick.. both qschematic and gpds has the same issue.

Yeah, I indeed think that those add_library(ALIAS) calls need to be moved. I Just didn't have time yet to investigate properly. See #53.

@bjornstromberg Do you want to provide a patch or shall I take care of it?

bjornstromberg commented 6 months ago

@Tectu to fix support for superbuild cmake layout just adding add_library({gpds,qschematic}::${target} ALIAS ${target}) in function(setup_target_common target) should do the trick.. both qschematic and gpds has the same issue.

Yeah, I indeed think that those add_library(ALIAS) calls need to be moved. I Just didn't have time yet to investigate properly. See #53.

@bjornstromberg Do you want to provide a patch or shall I take care of it?

@Tectu had totally missed #53 even though it was referenced in the post i quoted :roll_eyes: , right now I'm kind of low on free time, so if you have time to fix this it would probably get done sooner.

Tectu commented 6 months ago

@bjornstromberg Are you 100% sure that placing add_library({gpds,qschematic}::${target} ALIAS ${target}) in function(setup_target_common target) is the canonical way of handling this? I remember that when I first started using CMake many, many years ago I ran into some trouble with that but I don't recall the details - And I obviously never fixed it. Thinking about it nowdays, I guess there is not much preventing you from creating ALIAS libraries as much as you like.

Not doubting you btw - just checking whether this is a guess or a fact :p

bjornstromberg commented 6 months ago

@Tectu its a while since i wrote a superbuild that also had support for modular build and install, but i usually mirror the same wording that i use on the exported target, in this case qschematic::qschematic-static or -shared and in gpds, its gpds::gpds-shared or static depending on the target to use them via superbuilds while still being able to install them if that path is selected.

the reason for this is that when you export the alias during build, you put in a switch around find_package, it will have to look for if(NOT TARGET qschematic::qschematic-static (or shared..) and then it will run the find_package(QSchematic ...) in the project that uses qschematic, and qschematic does the same around find_package for getting gpds.

if you still run the find_package(QSchematic) in the derived project using qschematic, cmake will go bananas in different ways, depending on if you have a build dir setup and does an incremental build or a clean one or having the damn config file available in the CMAKE_INSTALL_PREFIX that can be used as a staging prefix. (kdevelop does it that way) at the correct time (data race when the file is not yet installed etc). there is even a few variants that actually works but not every time due to residual files on incremental builds..

anyway yes it is the way to do it, and its one of those things i never really documented all the ways its possible to shoot myself in the foot with in the black book since i did not write that book until years after i wrote those templates, so its on my todo list to actually document this, but there is like 10 different ways to mess this up that throws different errors, so my intention was to actually run test on all the ways to document it.

bjornstromberg commented 6 months ago

I will setup an env to fix this tonight when kids n wife sleeps. Should not take all night..

Tectu commented 6 months ago

Should not take all night™

bjornstromberg commented 6 months ago

Should not take all night™

probably also a great quote to frame and put on the wall..

bjornstromberg commented 6 months ago

waiting on build - now i remember why i stopped using superbuilds, and started embracing custom packaging of modules.. :roll_eyes:

simple testdriver:

CMakeLists.txt

cmake_minimum_required(VERSION 3.20)
project(superbuild)

add_subdirectory(gpds)
add_subdirectory(qschematic)

add_executable(${PROJECT_NAME}-static main.cxx)
target_link_libraries(${PROJECT_NAME}-static PRIVATE qschematic::qschematic-static)

add_executable(${PROJECT_NAME}-shared main.cxx)
target_link_libraries(${PROJECT_NAME}-shared PRIVATE qschematic::qschematic-shared)

main.cxx

#include <qschematic/view.hpp>
#include <gpds/container.hpp>

int
main(int /* argc */, char ** /* argv */)
{
  // * this test-file only tests so includes and linking works as expected * //

  gpds::container gpds_container; // access to gpds works

  QSchematic::View view; // access to qschematic works

  // wont test qt since qschematic works, qt implicitly works.

  return 0; // done..
}

gpds-fix-superbuild-support-alias.patch

From 01d5188129a43ef4c7fd6b8393c2f00cc61f8c26 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Str=C3=B6mberg?= <bjorn.stromberg86@gmail.com>
Date: Mon, 12 Feb 2024 20:20:00 +0100
Subject: [PATCH] cmake: add support for cmake superbuild

---
 lib/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
index 61b33f9..9bff656 100644
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -59,6 +59,9 @@ function(setup_target_common target)
            cxx_std_20
    )

+   # add alias to support cmake superbuild pattern
+   add_library(gpds::${target} ALIAS ${target})
+
    target_sources(
        ${target}
        PRIVATE
-- 
2.43.0

qschematic-fix-superbuild-support-alias.patch

From b67d8d766d2f384641135fb9b3a9a73f0284a3b0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Str=C3=B6mberg?= <bjorn.stromberg86@gmail.com>
Date: Mon, 12 Feb 2024 20:25:23 +0100
Subject: [PATCH 1/3] cmake: add support for cmake superbuild

---
 qschematic/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qschematic/CMakeLists.txt b/qschematic/CMakeLists.txt
index 7e80a3a..7e5aa88 100644
--- a/qschematic/CMakeLists.txt
+++ b/qschematic/CMakeLists.txt
@@ -23,6 +23,9 @@ function(setup_target_common target)
             cxx_std_20
     )

+    # add alias to support cmake superbuild pattern
+    add_library(qschematic::${target} ALIAS ${target})
+
     target_sources(
         ${target}
         PUBLIC
-- 
2.43.0

From e48b9710668863fbb73663e2b2b83f93e00ab1bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Str=C3=B6mberg?= <bjorn.stromberg86@gmail.com>
Date: Mon, 12 Feb 2024 20:44:51 +0100
Subject: [PATCH 2/3] cmake: add the guard flag if find_package(gpds) is
 required. to support cmake superbuild pattern

---
 qschematic/external.cmake | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/qschematic/external.cmake b/qschematic/external.cmake
index 80aae6c..133fee6 100644
--- a/qschematic/external.cmake
+++ b/qschematic/external.cmake
@@ -22,11 +22,14 @@ if (QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD)
         add_library(gpds::gpds-shared ALIAS gpds-shared)
     endif()
 else()
-    find_package(
-        gpds
-        ${QSCHEMATIC_DEPENDENCY_GPDS_MINIMUM_VERSION}
-        REQUIRED
-    )
+    # support superbuild where the gpds target might already be available
+    if(NOT TARGET ${QSCHEMATIC_DEPENDENCY_GPDS_TARGET})
+        find_package(
+            gpds
+            ${QSCHEMATIC_DEPENDENCY_GPDS_MINIMUM_VERSION}
+            REQUIRED
+        )
+    endif()
 endif()

-- 
2.43.0

From 7ac3d09b2dd883d49e332b03417eed3128e60124 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Str=C3=B6mberg?= <bjorn.stromberg86@gmail.com>
Date: Mon, 12 Feb 2024 21:05:38 +0100
Subject: [PATCH 3/3] cmake: fixing issue #53 these aliases should be created
 by gpds build script, it has been patched also in the patch sets published in
 #63

---
 qschematic/external.cmake | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/qschematic/external.cmake b/qschematic/external.cmake
index 133fee6..7647484 100644
--- a/qschematic/external.cmake
+++ b/qschematic/external.cmake
@@ -16,10 +16,6 @@ if (QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD)
         set(GPDS_BUILD_EXAMPLES OFF CACHE INTERNAL "")
         set(GPDS_FEATURE_SPDLOG OFF CACHE INTERNAL "")
         add_subdirectory(${gpds_SOURCE_DIR} ${gpds_BINARY_DIR})
-
-        # Create alias libraries
-        add_library(gpds::gpds-static ALIAS gpds-static)
-        add_library(gpds::gpds-shared ALIAS gpds-shared)
     endif()
 else()
     # support superbuild where the gpds target might already be available
-- 
2.43.0

first test build done via superbuild (using -DQSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD=OFF -DGPDS_DEPENDENCY_TINYXML2_PKGCONFIG=ON overrides to handle the tinyxml2 issue on alpine) finished cleanly, but i forgot to remove my qschematic & gpds packages so will run a second just to see so the flags around find_package is not required. they used to be required way back in 3.5 but that was a while ago..

bjornstromberg commented 6 months ago

yep the flag is still required. updating the patches soon.

updated the patch above, when this round has run, its just a quick test so both gpds and qschematic still builds with the patches applied.

bjornstromberg commented 6 months ago

well everything seems to work with superbuild and modular builds, only thing left to test is the fetch-content route. uncertain how the tinyxml2 issue will handle that case, but i guess we're about to find out in a short while..

bjornstromberg commented 6 months ago

doh.. fetch content cant be tested before the gpds patch above is applied on master, and it defaults to gpds 1.8.1 (QSCHEMATIC_DEPENDENCY_GPDS_MINIMUM_VERSION) so well as far as i can get this seems to work fine.

anyway did not actually take all night.. so i guess you take the night shift @Tectu to clean up this mess I've made :roll_eyes:

bjornstromberg commented 6 months ago

nobody took the Night Shift™ thats a improvement in the long run..

Tectu commented 6 months ago

@bjornstromberg Can you explain (in words) why the guard flag added in PATCH 2/3] cmake: add the guard flag if find_package(gpds) is is necessary?

doh.. fetch content cant be tested before the gpds patch above is applied on master, and it defaults to gpds 1.8.1 (QSCHEMATIC_DEPENDENCY_GPDS_MINIMUM_VERSION) so well as far as i can get this seems to work fine.

I was honestly wondering how long it would take until somebody would start complaining about that :D Personally, I feel the easiest option is to add QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD_VERSION and consume it in find_package(gpds).

bjornstromberg commented 6 months ago

@bjornstromberg Can you explain (in words) why the guard flag added in PATCH 2/3] cmake: add the guard flag if find_package(gpds) is is necessary?

the guard flag is so in case the gpds::gpds-static is already defined, it wont go down there and mess the build up with searching for the cmake package.

if it goes down there when superbuild already have defined it a) a staging or system installed gpds might pollute the superbuild with wrong gpds, this one might work and it might fail problem is unless you record git data into the binary you might think your using another version then you do... b) if staging tree and system does not have gpds installed so cmake can find the Config.cmake output, the REQUIRED keyword will break the build process.

doh.. fetch content cant be tested before the gpds patch above is applied on master, and it defaults to gpds 1.8.1 (QSCHEMATIC_DEPENDENCY_GPDS_MINIMUM_VERSION) so well as far as i can get this seems to work fine.

I was honestly wondering how long it would take until somebody would start complaining about that :D Personally, I feel the easiest option is to add QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD_VERSION and consume it in find_package(gpds).

go for it!

i personally don't use the fetch content feature of cmake for these kinds of unmaintainable things.

edit: unmaintainable as in overrides required all over the place to be able to test it..

Tectu commented 6 months ago

Pardon my potential ignorance but I am still not getting it. I mean, I fully understand what you're saying, but I must be missing a crucial piece of information here preventing me from actually understanding it. The whole point of QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD is to give the user control over whether to superbuild or not to superbuild. When QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD is set to OFF, it will do find_package(). In which scenario would gpds::gpds-target already be defined without calling find_package() first?

Generally, I agree with you: Superbuilds are a pain in the rear. I added this capability to QSchematic because I don't want potential users to be discouraged from playing around with the library due to a "build & install this first"-hurdle.

bjornstromberg commented 6 months ago

Pardon my potential ignorance but I am still not getting it. I mean, I fully understand what you're saying, but I must be missing a crucial piece of information here preventing me from actually understanding it.

nah i think we're having a breakdown in communication..
i thought wou wanted QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD_VERSION to control the fetch_content target tag.
this which would also require a target repo override so i could test against my local repo,
which is why i think the whole fetch content is unmaintainable, to much variables to get out of hand.

The whole point of QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD is to give the user control over whether to superbuild or not to superbuild. When QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD is set to OFF, it will do find_package(). In which scenario would gpds::gpds-target already be defined without calling find_package() first?

in that case if i understand you correctly, QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD would need to be a flag ON, OFF or SUPERBUILD with three code paths.

if ON it use fetch content if OFF it use find_package else it assumes SUPERBUILD mode where gpds is a sub_directory of a parent CMake project.

generally superbuild can be detected at the start of the top level cmake project if that current cmake list file is the top cmake list file.

Generally, I agree with you: Superbuilds are a pain in the rear. I added this capability to QSchematic because I don't want potential users to be discouraged from playing around with the library due to a "build & install this first"-hurdle.

both fetch content, and superbuild are patterns that are good for some usecases but the costs/drawbacks are significant.
they work kind of like some offers, get you hooked before the bill comes due.. :roll_eyes:

ps. right now i need to focus on getting the home cinema fixed,
switched from broken upgrade: LibreElec-11.0.3 -> LibreElec-11.0.6 which had a NFS bug that took down the whole lan..
to running kodi on alpine linux. so my contributions on this issue will have to wait a few days.

Tectu commented 6 months ago

I think there was indeed some miscommunication. I have no intention of supporting superbuilds in form of "here's a manually managed subdirectory". The superbuild s facilitated via FetchContent_Declare(). This will automatically grab the dependency source and place it under cmake-build-dir/_deps. Therefore, I think this guard is really not needed. There are only ever two scenarios which are controlled by QSCHEMATIC_DEPENDENCY_GPDS_DOWNLOAD (either ON or OFF - it's a CMake option):

As such, I think the case that you're trying to guard against is not happening.

Tectu commented 6 months ago

Forgot to reference this issue in the commit: 778d8294e3df9402ac7a4e1cb2cea4eee8ea4350

Tectu commented 6 months ago

@TobiasWallner Could you test/try the latest master branch and tell us whether that works out of the box for you?

bjornstromberg commented 6 months ago

As such, I think the case that you're trying to guard against is not happening.

just add some info into the readmes of QSchematic and gpds that supported ways to consume the libaray is via modular build and fetch content. also note that embedding via sub foldering is not supported and not intended.

then we can just point all the issues to the readme and close them when people try to do that.
cause its the first way people 'learn' to play with cmake, so i usually tend to regard that 'dumb' use case as a limit the effort of issue ping pong.

edit: i know the information is already located in the readme, but if the 'overview' just lists the way to consume it, and also state the way that does not work, people might actually read it.

Tectu commented 6 months ago

@TobiasWallner We've released 1.6.0 in the meantime. It would be great if you could test that in your scenario & report back.

TobiasWallner commented 6 months ago

@Tectu GPDS and QSchematich both seem to build just fine.

Tectu commented 6 months ago

Glad to hear that - Thanks for reporting back!