2020-03-04 13-56-25 PST Sergei Izmailov Let's try. I've tested on 19.04 - works fine 2020-03-07 16-47-39 PST Sergei Izmailov 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 PST Sergei Izmailov Hi, did you have a chance to look at docker MR? 2020-03-30 15-31-36 PDT Sergei Izmailov In fact it's about same size, see screenshot above 2020-03-04 13-33-48 PST Sergei Izmailov Or you think moving gtest to minimal is better? 2020-03-26 09-30-29 PDT Sergei Izmailov ok, I'll rebase in a minute 2020-03-27 07-31-33 PDT Sergei Izmailov 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 PDT Sergei Izmailov It depends in which mode you build 2geom. 2020-03-07 16-50-47 PST Sergei Izmailov I don't like false-positive warnings. We can limit it to ubuntu 18.04 only 2020-03-19 14-58-05 PDT Sergei Izmailov No problem, take your time. Just checking. 2020-03-04 07-32-57 PST Sergei Izmailov 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 PST Sergei Izmailov 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 PDT Sergei Izmailov Sorry, I misread that. 2020-03-19 15-00-34 PDT Sergei Izmailov (i guess) 2020-03-19 14-20-26 PDT Sergei Izmailov 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 PDT Sergei Izmailov 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 PST Sergei Izmailov Should I ask Martin to do it? 2020-05-07 13-15-55 PDT Sergei Izmailov 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 PST Sergei Izmailov (`googletest` soruces is dependency and will be installed automatically ) 2020-03-30 15-29-08 PDT Sergei Izmailov libinkscape_base.dll.a (https://chat.inkscape.org/file-upload/JHxjTscxfaTZuRcZ3/libinkscape_base.dll...) 2020-03-04 13-44-24 PST Sergei Izmailov 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 PDT Sergei Izmailov 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 PST Sergei Izmailov Should I add it to MR? 2020-03-19 15-55-24 PDT Sergei Izmailov ``` 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 PDT Sergei Izmailov Does windows CI run only on master branch, but not on MRs? 2020-03-19 15-01-44 PDT Sergei Izmailov I can try to build inkscape against shared lib2geom 2020-03-30 15-31-14 PDT Sergei Izmailov 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 PST Sergei Izmailov I'm new to gitlab CI 2020-03-07 16-48-51 PST Sergei Izmailov Such test would add ~30 lines to `.sh` script 2020-03-15 04-22-15 PDT Sergei Izmailov 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 PST Sergei Izmailov There is no binaries 2020-03-04 13-31-49 PST Sergei Izmailov right? 2020-03-26 11-11-48 PDT Sergei Izmailov ... and it fails [link](https://ci.appveyor.com/project/inkscape/inkscape/builds/31745274/job/52b089...). I don't quite get it. `lib/libinkscape_base.dll.a` has defined symbols from `lib/lib2geom.a`... 2020-03-26 13-22-09 PDT Sergei Izmailov 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 PST Sergei Izmailov Seems like CI doesn't have google tests. Can you help with it ? 2020-03-04 13-59-12 PST Sergei Izmailov I don't think lib2geom uses mock. I have no guesses here 2020-03-19 15-03-38 PDT Sergei Izmailov 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 PST Sergei Izmailov Thanks for testing! 2020-03-07 16-53-53 PST Sergei Izmailov 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 PDT Sergei Izmailov Currently MR builds STATIC library (default setting in lib2geom). 2020-03-07 16-58-43 PST Sergei Izmailov 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 PDT Sergei Izmailov Hi, I think you still plan to give a review to MR, right? (Thomas seems to approved it) 2020-03-30 15-32-24 PDT Sergei Izmailov nm (https://chat.inkscape.org/file-upload/ty2TRYMN2pNSnCtCg/nm) 2020-03-07 16-50-59 PST Sergei Izmailov But managing all versions seems to be tedious 2020-03-30 15-34-21 PDT Sergei Izmailov `lib2geom.a` and `lininkscape_base.dll.a` provide same global symbol 2020-03-04 13-30-05 PST Sergei Izmailov How can i update the image? 2020-03-26 13-25-55 PDT Sergei Izmailov Damn, older cmake rejects it. Sounds like a task for tomorrow. 2020-03-04 13-21-36 PST Sergei Izmailov It's already there, but needs special flag.. I'm not sure how to proceed with it 2020-03-04 12-59-15 PST Sergei Izmailov 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....) ``` /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 PST Sergei Izmailov 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 PST Sergei Izmailov I've submitted a MR with extra build step for gtest 2020-05-07 07-08-52 PDT Sergei Izmailov 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 PST Sergei Izmailov I though about passing `--recommended` flag 2020-03-26 12-46-04 PDT Sergei Izmailov 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 PST Sergei Izmailov And it's the complaint from cmake at CI 2020-03-19 15-57-46 PDT Sergei Izmailov (I've added tests to lib2geom to build a super-project against it as installed and subdirectory library) 2020-03-07 16-51-50 PST Sergei Izmailov 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 PDT Sergei Izmailov is it desired? 2020-05-15 08-02-22 PDT Sergei Izmailov 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 PST Sergei Izmailov Oh, I see the https://gitlab.com/inkscape/inkscape-ci-docker/-/blob/master/Dockerfile 2020-03-26 09-56-02 PDT Sergei Izmailov Nevermind, I see AppVeyor started the job, but it doesn't appear as build stage on gitlab. 2020-03-07 17-00-13 PST Sergei Izmailov 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 PDT Sergei Izmailov I think it confirms that lib2geom can be packaged separately 2020-03-04 14-47-26 PST Sergei Izmailov 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-re...
I think I'll add instructions to build and install googletest to docker image 2020-03-04 13-59-57 PST Sergei Izmailov I think `libgtest-dev` is enough 2020-03-04 13-58-20 PST Patrick (but should still have everything needed to build) 2020-05-07 12-44-26 PDT Patrick 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 PDT Patrick 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 PDT Patrick 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 PDT Patrick 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 PDT Patrick If it's reasonable... We can obviously take this in steps as well. 2020-03-04 13-32-27 PST Patrick (which is called by the Dockerfile) 2020-03-04 07-53-52 PST Patrick 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 PDT Patrick But if we can isolate the subproject enough, it might come "for free". 2020-03-07 17-01-39 PST Patrick 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 PST Patrick Yes, if `CI` is set, build and install cmake, otherwise print a warning that recommends to do it. 2020-03-04 13-59-33 PST Patrick So we only need gtest? 2020-03-19 15-01-13 PDT Patrick I meant with your MR against Inkscape. 2020-03-04 13-34-16 PST Patrick CI has everything 2020-03-07 16-51-02 PST Patrick Yes, I think that's exactly what you were doing in your MR? 2020-03-15 04-38-37 PDT Patrick 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 PDT Patrick Maybe all you need to do is make cpp_test_static_library actually static? 2020-03-26 12-15-52 PDT Patrick Yep, only for tests, though. Do you link the test library differently from other internal libs? 2020-03-04 14-12-51 PST Patrick Great! Once we figure it out, we can try to simplify the logic in inkscape/inkscape as well 2020-03-26 10-18-06 PDT Patrick Yeah, GitLab is stupid that way... 2020-03-27 09-03-49 PDT Patrick .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 PST Patrick 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 PST Patrick OK, yeah, I guess we might only use headers for Inkscape. 2020-03-04 14-05-20 PST Patrick 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#... still makes it work (I'm pretty sure we don't even need all of it) 2020-03-07 17-01-56 PST Patrick Not even all people might use the shell script after all... 2020-03-26 12-19-02 PDT Patrick (you might want to check if static build still works btw, too. It caused problems in the past) 2020-03-19 15-03-14 PDT Patrick (although it's probably not too helpful right now) 2020-03-15 05-05-11 PDT Patrick np, thanks for checking the code! 2020-03-26 12-17-41 PDT Patrick Ah, I think tests are linked statically against libinkscape-base with your changes? 2020-03-04 10-05-53 PST Patrick 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 PDT Patrick It's not fixed yet but uses a shared library of 2geom as workaround 2020-03-26 12-23-37 PDT Patrick (or idd that fail as well) 2020-03-04 13-55-12 PST Patrick I could check locally if that helps (I have a Ubuntu VM 18.04 set-up). 2020-03-19 15-00-56 PDT Patrick (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 PDT Patrick At some point the hope was to get lib2geom packaged separately from Inkscape. 2020-03-26 09-04-59 PDT Patrick 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 PDT Patrick No, Module mode is the default: 2020-03-07 16-55-09 PST Patrick 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 PST Patrick 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 PST Patrick 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 PDT Patrick Feel free to rebase your MR to trigger a new CI build, otherwise I can do so later. 2020-03-26 12-20-15 PDT Patrick 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 PST Patrick Create an MR with your changes as usual. 2020-03-07 15-58-28 PST Patrick 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 PDT Patrick 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 PST Patrick We can add what we need here: https://gitlab.com/inkscape/inkscape-ci-docker/-/blob/master/install_depende... 2020-05-07 13-43-15 PDT Patrick 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 PST Patrick (`--full`) 2020-03-19 15-01-22 PDT Patrick (the default behavior) 2020-03-04 15-06-54 PST Patrick The files you're missing seem to be in `googletest`: https://packages.ubuntu.com/bionic/amd64/googletest/filelist 2020-03-04 13-32-01 PST Patrick no, the file I linked. 2020-03-04 13-58-00 PST Patrick 18.04 is different. I think it misses something gmock-related.