2020-03-04 13-56-25 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Let's try. I've tested on 19.04 - works fine
2020-03-07 16-47-39 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I think we can test gtest installation via minimal cmake project and provide instructions to fix that. A minimal cmake project would contain single `CMakeLists.txt` file: ```cmake cmake_minimum_required(VERSION 3.0) project(temp) find_package(GTest REQUIRED MODULE) ```
2020-03-07 06-44-00 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Hi, did you have a chance to look at docker MR?
2020-03-30 15-31-36 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>In fact it's about same size, see screenshot above
2020-03-04 13-33-48 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Or you think moving gtest to minimal is better?
2020-03-26 09-30-29 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>ok, I'll rebase in a minute
2020-03-27 07-31-33 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Do you know is `lib/libinkscape_base.dll.a` shared or static? The line ``` [984/1057] Linking CXX shared library bin\libinkscape_base.dll ``` suggests it's shared, but name and location are different. Unfortunately I'm not in a position to test on windows at the moment. I don't see how `2geom` got included in `libinkscape_base`. I'll try to investigate this topic in more detail later (maybe in a week or so). I could experiment with AppVeyor, but it's too long and makes others wait a lot. Currently it doesn't compile even without extra OBJECT/STATIC/SHARED test library. Please share your ideas if you have some
2020-03-19 15-00-14 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>It depends in which mode you build 2geom.
2020-03-07 16-50-47 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I don't like false-positive warnings. We can limit it to ubuntu 18.04 only
2020-03-19 14-58-05 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>No problem, take your time. Just checking.
2020-03-04 07-32-57 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Hi! Can you give a comment on https://gitlab.com/inkscape/lib2geom/-/merge_requests/19 ? (if you prefer we can discuss it here or there)
2020-03-04 08-22-54 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I appreciate that this change is not urgent and can be delayed to post 1.0 (I remember you said that, probably it was in chat, can't find it in MRs). I think you are reasonable person and your suggestions has ho have something behind, but I don't see it clearly so far. I didn't ignore you questions (at least intentionally). Please point out/copy-paste. I didn't argue against any extra changes just because I'm lazy to address your comments. I want to find best solution of possible. You can read that I'm not 100% with my arguments and my comment leaves question open to discuss. Please don't silence conversations as it's not a productive way to solve problems (as well as over-communication). If you think that I'm wrong/lazy/not in right time, please tell so. Again, if it's just lack of time, lets hold it until 1.0 release. It's totally fine by me.
2020-03-15 04-40-18 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Sorry, I misread that.
2020-03-19 15-00-34 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>(i guess)
2020-03-19 14-20-26 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Sorry for bothering you again. Is there is any problems with https://gitlab.com/inkscape/inkscape/-/merge_requests/1410 except lack of dev time?
2020-05-07 13-54-24 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>I see. (To be honest I feel uneasy with windows as developer, especially when it comes as low level as like linking. I think I can utilize my time better fixing other stuff.)
2020-03-04 13-30-21 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Should I ask Martin to do it?
2020-05-07 13-15-55 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>I'm a unix person, so I don't know much about Windows stuff. All I found I wrote here.
2020-03-04 14-00-17 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>(`googletest` soruces is dependency and will be installed automatically )
2020-03-30 15-29-08 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>libinkscape_base.dll.a (https://chat.inkscape.org/file-upload/JHxjTscxfaTZuRcZ3/libinkscape_base.dll.a)
2020-03-04 13-44-24 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I don't understand. All seems to be installed, but it's not found by cmake for some reason https://gitlab.com/inkscape/inkscape-ci-docker/-/jobs/455010902
2020-03-15 06-31-15 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>It's all case-specific. Some libraries act similar in both mods, some don't (like gtest). Explicit specification of MODULE/CONFIG prohibits use of another one. I think generally CONFIG is more preferable since it's mostly auto-generated and less error prone, but it requires some efforts from library authors so it's not widespread. At the moment I agree, let's keep it as is.
2020-03-07 16-49-06 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Should I add it to MR?
2020-03-19 15-55-24 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>``` set(2GEOM_BUILD_SHARED ON CACHE BOOL "Build 2geom as SHARED library") add_subdirectory(2geom) ``` is enough to build and link against shared version of lib2geom. I've build with `-DCMAKE_INSTALL_PREFIX:PATH=~/.local/` so shared library is not found with default settings, therefore `~/.local/bin/inkscape` can't be launched without `LD_LIBRARY_PATH` or `LD_PRELOAD`.
2020-03-26 09-54-16 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Does windows CI run only on master branch, but not on MRs?
2020-03-19 15-01-44 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>I can try to build inkscape against shared lib2geom
2020-03-30 15-31-14 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>I'm not really familiar with "import libraries", but from what I read it should be quite small compared to shared library
2020-03-04 13-22-13 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I'm new to gitlab CI
2020-03-07 16-48-51 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Such test would add ~30 lines to `.sh` script
2020-03-15 04-22-15 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Hi! I've noticed you simplified GTest handling in main repo. The `find_package` call must include `MODULE` keyword since "config" mode takes precedence over "module" mode and we are using "module" features. It will fail on systems with properly installed GTest.
2020-03-04 15-07-21 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>There is no binaries
2020-03-04 13-31-49 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>right?
2020-03-26 11-11-48 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>... and it fails [link](https://ci.appveyor.com/project/inkscape/inkscape/builds/31745274/job/52b089vq28ln1wld#L2889). I don't quite get it. `lib/libinkscape_base.dll.a` has defined symbols from `lib/lib2geom.a`...
2020-03-26 13-22-09 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>I've dropped my previous commits to `testfiles/CMakeLists.txt` and added extra `target_link_libraries` for OBJECT library you created. It worked locally, let's see how would CI go
2020-03-04 13-17-37 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Seems like CI doesn't have google tests. Can you help with it ?
2020-03-04 13-59-12 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I don't think lib2geom uses mock. I have no guesses here
2020-03-19 15-03-38 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Currently I think it's ready, it's matter of one line change. I'm going to check it right now.
2020-03-04 14-07-52 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Thanks for testing!
2020-03-07 16-53-53 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I think warning should be issued at the very end of script. Otherwise it's easy to overlook it in ton of messages
2020-03-19 16-04-16 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Currently MR builds STATIC library (default setting in lib2geom).
2020-03-07 16-58-43 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I thought about it. It's better since it's never false-positive, but I don't like the idea of fixing it in CMakeLists...
2020-03-26 07-47-44 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Hi, I think you still plan to give a review to MR, right? (Thomas seems to approved it)
2020-03-30 15-32-24 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>nm (https://chat.inkscape.org/file-upload/ty2TRYMN2pNSnCtCg/nm)
2020-03-07 16-50-59 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>But managing all versions seems to be tedious
2020-03-30 15-34-21 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>`lib2geom.a` and `lininkscape_base.dll.a` provide same global symbol
2020-03-04 13-30-05 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>How can i update the image?
2020-03-26 13-25-55 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Damn, older cmake rejects it. Sounds like a task for tomorrow.
2020-03-04 13-21-36 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>It's already there, but needs special flag.. I'm not sure how to proceed with it
2020-03-04 12-59-15 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I'm sorry. Sometimes it's hard to explain myself properly. (In process of re-explaining I think I solved the googletest issue) 1. You are right, the difference between 2.8 and 3.0 styles is irrelevant here. BTW here is a good talk about modern cmake style and why we should avoid "2.8-style" https://youtu.be/bsXLMQ6WgIk. 2. The problem with `googletest`/`libgtest-dev`/`libmock-dev` packages on Ubuntu: None of them installs following files (compare to RPM: http://rpmfind.net/linux/RPM/fedora/devel/rawhide/aarch64/g/gtest-devel-1.8.1-5.fc32.aarch64.html) ``` /usr/lib64/cmake/GTest/GTestConfig.cmake /usr/lib64/cmake/GTest/GTestConfigVersion.cmake /usr/lib64/cmake/GTest/GTestTargets-noconfig.cmake /usr/lib64/cmake/GTest/GTestTargets.cmake ``` (pkg-config files are also missing but we are not interested in them now) They contain information about build targets and their flags. Without them we need to repeat this information ourself. Fortunately cmake-shipped [FindGTest](https://cmake.org/cmake/help/latest/module/FindGTest.html) exists and does it for us. `find_package(GTest)` can find headers and libraries and resurrect all required flags (https://cmake.org/cmake/help/latest/module/FindGTest.html) and even define `IMPROTED` targets `GTest::GTest` and `GTest::Main`. The only issue it has it names googletest targets differently from googletest itself. But it's not a big deal, we can ignore generated cmake files (`GTestConfig.cmake`) on system where googletest installed properly. I guess some of `googletest`/`libgtest-dev`/`libmock-dev` are available on CI, right?
2020-03-04 14-07-31 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I would prefer to stick to simpler solution. I think I'll do tests on 18.04 on my own to see what exactly goes wrong with bare `find_package`
2020-03-04 15-45-24 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I've submitted a MR with extra build step for gtest
2020-05-07 07-08-52 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Hi, congrats on 1.0 release! Can you find time to look at https://gitlab.com/inkscape/inkscape/-/merge_requests/1410 problem ?
2020-03-04 13-33-13 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I though about passing `--recommended` flag
2020-03-26 12-46-04 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>I've tried static/shared and ended up with no library at all. I guess your intention was to avoid recompiling of those two files... Let's see if it would work. Probably you initial approach would also work (i.e. with OBJECT library) with repeated `target_link_libraries` for OBJECT library and tests which uses it.
2020-03-04 15-07-48 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>And it's the complaint from cmake at CI
2020-03-19 15-57-46 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>(I've added tests to lib2geom to build a super-project against it as installed and subdirectory library)
2020-03-07 16-51-50 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Currently I don't have any warnings. You suggest to add warning only to non-CI ubuntu 18.04 ?
2020-03-19 15-02-07 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>is it desired?
2020-05-15 08-02-22 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Hi, do you know how windows build was fixed? Just curious. Don't see obvious changes to cmake files
2020-03-04 13-31-47 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Oh, I see the https://gitlab.com/inkscape/inkscape-ci-docker/-/blob/master/Dockerfile
2020-03-26 09-56-02 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>Nevermind, I see AppVeyor started the job, but it doesn't appear as build stage on gitlab.
2020-03-07 17-00-13 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I'll add warning to `.sh` script. Let's think about cmake warning a bit more and decide later
2020-03-19 15-56-27 PDTSergei Izmailov <sergei.a.izmailov@gmail.com>I think it confirms that lib2geom can be packaged separately
2020-03-04 14-47-26 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>Looks like `libgtest-dev` in 18.04 is really poor. Compare 18.04 and 19.04: https://packages.ubuntu.com/disco/amd64/libgtest-dev/filelist https://packages.ubuntu.com/bionic/amd64/libgtest-dev/filelist We are not alone with this problem: https://stackoverflow.com/questions/24295876/cmake-cannot-find-googletest-required-library-in-ubuntu I think I'll add instructions to build and install googletest to docker image
2020-03-04 13-59-57 PSTSergei Izmailov <sergei.a.izmailov@gmail.com>I think `libgtest-dev` is enough
2020-03-04 13-58-20 PSTPatrick <eduard.braun2@gmx.de>(but should still have everything needed to build)
2020-05-07 12-44-26 PDTPatrick <eduard.braun2@gmx.de>I will, but I can't promise you when. Did you make progress on the linking issue on Windows or do we still need to solve that?
2020-03-15 05-07-58 PDTPatrick <eduard.braun2@gmx.de>It might even be a good idea to explicitly specify module mode everywhere, but I don't think we do that anywhere yet, so it seemed safer to just keep it as-is (especially as it should not matter if the module is properly found in module mode as expected)
2020-03-19 14-59-47 PDTPatrick <eduard.braun2@gmx.de>Maybe one question ahead of time: Do we link lib2geom dynamically in your MR? Because that certainly would be a goal going forward.
2020-03-19 14-57-26 PDTPatrick <eduard.braun2@gmx.de>Well, I can't tell you if there are any problems before I have reviewed it (which I plan to do, eventually, but yeah I'm backlogged).
2020-03-19 15-02-09 PDTPatrick <eduard.braun2@gmx.de>If it's reasonable... We can obviously take this in steps as well.
2020-03-04 13-32-27 PSTPatrick <eduard.braun2@gmx.de>(which is called by the Dockerfile)
2020-03-04 07-53-52 PSTPatrick <eduard.braun2@gmx.de>You seem to be ignoring my comments (everything I said so far you've not replied to at all or argued against, giving me the impression that you're not really willing to actually change anything) so I'm not overly motivated to spend any more time on that. (In fact the very first thing you ignored was my suggestion to move this to post-1.0 when people will have more time to review properly and it will be less intrusive to make larger changes to the build system).
2020-03-19 15-03-35 PDTPatrick <eduard.braun2@gmx.de>But if we can isolate the subproject enough, it might come "for free".
2020-03-07 17-01-39 PSTPatrick <eduard.braun2@gmx.de>OK... I just would not like to add a lot of stuff to `.sh` script for a workaround for one specific version of one specific OS either, so let's keep it minimal (especially considering it might not be seen by all people)
2020-03-07 16-52-56 PSTPatrick <eduard.braun2@gmx.de>Yes, if `CI` is set, build and install cmake, otherwise print a warning that recommends to do it.
2020-03-04 13-59-33 PSTPatrick <eduard.braun2@gmx.de>So we only need gtest?
2020-03-19 15-01-13 PDTPatrick <eduard.braun2@gmx.de>I meant with your MR against Inkscape.
2020-03-04 13-34-16 PSTPatrick <eduard.braun2@gmx.de>CI has everything
2020-03-07 16-51-02 PSTPatrick <eduard.braun2@gmx.de>Yes, I think that's exactly what you were doing in your MR?
2020-03-15 04-38-37 PDTPatrick <eduard.braun2@gmx.de>> If the MODULE option is not specfied in the above signature, CMake first searches for the package using Module mode. Then, if the package is not found, it searches again using Config mode.
2020-03-26 12-23-14 PDTPatrick <eduard.braun2@gmx.de>Maybe all you need to do is make cpp_test_static_library actually static?
2020-03-26 12-15-52 PDTPatrick <eduard.braun2@gmx.de>Yep, only for tests, though. Do you link the test library differently from other internal libs?
2020-03-04 14-12-51 PSTPatrick <eduard.braun2@gmx.de>Great! Once we figure it out, we can try to simplify the logic in inkscape/inkscape as well
2020-03-26 10-18-06 PDTPatrick <eduard.braun2@gmx.de>Yeah, GitLab is stupid that way...
2020-03-27 09-03-49 PDTPatrick <eduard.braun2@gmx.de>.dll.a indicates a so-called "import library". I think with gcc we can link directly against the .dll though. (so this is probably where the problem comes from: linking directly against libinkscape_base.dll once and indirectly linking against it a second time via the .dll.a)
2020-03-07 16-50-05 PSTPatrick <eduard.braun2@gmx.de>nah, that seems like overkill... I'd suggest to simply print a warning if `CI` is unset (same conditions otherwise).
2020-03-04 15-28-38 PSTPatrick <eduard.braun2@gmx.de>OK, yeah, I guess we might only use headers for Inkscape.
2020-03-04 14-05-20 PSTPatrick <eduard.braun2@gmx.de>Ok, so `find_package` fails locally as well. However the logic in https://gitlab.com/inkscape/inkscape/-/blob/master/CMakeLists.txt#L177-197 and https://gitlab.com/inkscape/inkscape/-/blob/master/testfiles/CMakeLists.txt#L5-13 still makes it work (I'm pretty sure we don't even need all of it)
2020-03-07 17-01-56 PSTPatrick <eduard.braun2@gmx.de>Not even all people might use the shell script after all...
2020-03-26 12-19-02 PDTPatrick <eduard.braun2@gmx.de>(you might want to check if static build still works btw, too. It caused problems in the past)
2020-03-19 15-03-14 PDTPatrick <eduard.braun2@gmx.de>(although it's probably not too helpful right now)
2020-03-15 05-05-11 PDTPatrick <eduard.braun2@gmx.de>np, thanks for checking the code!
2020-03-26 12-17-41 PDTPatrick <eduard.braun2@gmx.de>Ah, I think tests are linked statically against libinkscape-base with your changes?
2020-03-04 10-05-53 PSTPatrick <eduard.braun2@gmx.de>Lack of time is a factor: If I had time to spare I would try to make the changes I suggested in your branch myself to see if they're viable. However as I'm currently spending my time on release-relevant things, I can only make the suggestion and have to depend on you to try to implement them. I'm not silencing anything, but if the feedback from you is "I don't see the need for a change" there's nothing I can work with (my original suggestion still stands, but I can not force you to try and implement it, and I currently won't spend time in it myself.)
2020-05-15 09-01-11 PDTPatrick <eduard.braun2@gmx.de>It's not fixed yet but uses a shared library of 2geom as workaround
2020-03-26 12-23-37 PDTPatrick <eduard.braun2@gmx.de>(or idd that fail as well)
2020-03-04 13-55-12 PSTPatrick <eduard.braun2@gmx.de>I could check locally if that helps (I have a Ubuntu VM 18.04 set-up).
2020-03-19 15-00-56 PDTPatrick <eduard.braun2@gmx.de>(it's insanity that we link everything into one huge library right now - it's even causing problems, e.g. we can't currently do debug builds on Windows as there are too many symbols in the library)
2020-03-19 15-02-53 PDTPatrick <eduard.braun2@gmx.de>At some point the hope was to get lib2geom packaged separately from Inkscape.
2020-03-26 09-04-59 PDTPatrick <eduard.braun2@gmx.de>I looked at it and from reading the diff it should be fine. The primary thing I wanted to check was whether Windows CI passes (as I did not have the time to build locally yet). Unfortunately Windows CI was broken a while due to a row of broken updates in glibmm and mingw-w64 itself. Luckily that's all sorted out now.
2020-03-15 04-38-24 PDTPatrick <eduard.braun2@gmx.de>No, Module mode is the default:
2020-03-07 16-55-09 PSTPatrick <eduard.braun2@gmx.de>That's true... if we want to ensure people see it we should probably add the waring to 2geom's CMakeLists.txt (and show it once `find_package(GTest REQUIRED MODULE)` fails)
2020-03-04 10-14-18 PSTPatrick <eduard.braun2@gmx.de>Wrt to the last sugestion: I'm not sure what your comment with "old style cmake 2.8" is referring to (I did suggest no such thing). What I suggested was to investigate _why_ the Ubuntu googletest/gmock packages are not found by the CMake module (I think you already have a rough idea) and attempt to fix the detection of the package rather than to download and build it manually.
2020-03-04 13-29-39 PSTPatrick <eduard.braun2@gmx.de>Well, let me know if/how I can help. We might be missing "libgmock-dev" (not available on Ubuntu 18.04 as it seems)
2020-03-26 09-05-58 PDTPatrick <eduard.braun2@gmx.de>Feel free to rebase your MR to trigger a new CI build, otherwise I can do so later.
2020-03-26 12-20-15 PDTPatrick <eduard.braun2@gmx.de>Oh, and I don't like https://gitlab.com/inkscape/inkscape/-/commit/e31698fb - I've removed that for a reason ;-)
2020-03-04 13-31-03 PSTPatrick <eduard.braun2@gmx.de>Create an MR with your changes as usual.
2020-03-07 15-58-28 PSTPatrick <eduard.braun2@gmx.de>I'll have a look tomorrow. Can we give general advice for affected users? (Possibly we could even print a warning explaining what to do?)
2020-05-07 13-41-15 PDTPatrick <eduard.braun2@gmx.de>Well, I'm a Windows person, but that usually doesn't rule out that I try to fix Unix stuff ;-)
2020-03-04 13-20-56 PSTPatrick <eduard.braun2@gmx.de>We can add what we need here: https://gitlab.com/inkscape/inkscape-ci-docker/-/blob/master/install_dependencies.sh
2020-05-07 13-43-15 PDTPatrick <eduard.braun2@gmx.de>It's OK if you don't want to try to fix the Windows part further , but I'm not sure when I find the time to investigate it myself.
2020-03-04 13-34-30 PSTPatrick <eduard.braun2@gmx.de>(`--full`)
2020-03-19 15-01-22 PDTPatrick <eduard.braun2@gmx.de>(the default behavior)
2020-03-04 15-06-54 PSTPatrick <eduard.braun2@gmx.de>The files you're missing seem to be in `googletest`: https://packages.ubuntu.com/bionic/amd64/googletest/filelist
2020-03-04 13-32-01 PSTPatrick <eduard.braun2@gmx.de>no, the file I linked.
2020-03-04 13-58-00 PSTPatrick <eduard.braun2@gmx.de>18.04 is different. I think it misses something gmock-related.