ned14 / outcome

Provides very lightweight outcome<T> and result<T> (non-Boost edition)
https://ned14.github.io/outcome
Other
676 stars 62 forks source link

Built & installed cmake target outcome::hl missing quickcpplib::hl #230

Closed cneumann closed 4 years ago

cneumann commented 4 years ago

I've previously used the outcome version that comes with vcpkg [1,2], but ran into issues using that in c++20 mode on Visual Studio 2019. Now I'm trying to switch to using the develop branch.

Using a Visual Studio 2019 command prompt:

git clone https://gitihub.com/ned14/outcome
cd outcome && git submodule init && git submodule update --recursive
cd .. && md build && cd build
cmake -G "Visual Studio 16 2019" -A x64 -CMAKE_INSTALL_PREFIX=%CD%\..\install ..\outcome
cmake --build . --config Release
cmake --build . --config Release --target INSTALL

So far so good, the headers and in %CMAKE_INSTALL_PREFIX%\include\outcome and outcomeConfig.cmake in %CMAKE_INSTALL_PREFIX%\lib\cmake\outcome. I'm getting a bunch of python syntax errors during the build step. Not sure if that is related, see below for details on those [3].

Now trying to build a simple test program with a CMakeLists.txt like this:

cmake_minimum_required(VERSION 3.17)
project(testoutcome)
find_package(outcome CONFIG REQUIRED)
add_executable("testoutcome" "main.cpp")
target_link_libraries("testoutcome" PRIVATE outcome::hl)
target_compile_features("testoutcome" PUBLIC cxx_std_20)
set_property(TARGET "testoutcome" PROPERTY CXX_STANDARD_REQUIRED ON)

Running cmake -G "Visual Studio 16 2019" -A x64 -Doutcome_DIR=%PREFIX%\install\lib\cmake\outcome ..\testoutcome Fails with

CMake Error at CMakeLists.txt:11 (add_executable):
  Target "testoutcome" links to target "quickcpplib::hl" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?

quickcpplib got cloned and built into the build directory during the configure phase (first cmake invocation) of building outcome - which was a little surprising, but okay. Anyway, the automagic seems to stop there as it wasn't installed along outcome and that causes the cmake error for my test program. If quickcpplib is a vendored dependency of outcome wouldn't it be more conventional to have it as a git submodule or just something I have to have installed before building outcome? I know outcome is header only and I don't really have to "build" it and could just copy the single header version somewhere on my compilers include search path, but in general I prefer having my dependencies show up as find_package lines and make them available to targets by listing them in target_link_libraries. That way I don't accidentally start using headers that just happen to be on the search path. I guess what I'm asking boils down to: is there a mode of the build that doesn't try to be so clever about dependencies and just fails if quickcpplib is not found via the usual cmake mechanisms? Thanks!

[1] https://github.com/microsoft/vcpkg [2] I believe vcpkg just comes with outcome v2.1 [3] Python syntax errors seen during build (this is with python 3.7.7)

include/outcome/config.hpp : 128 warning : couldn't evaluate expression due to Traceback (most recent call last): [C:\Users\cneumann\pers
onal\outcome\build\outcome_hl-pp-abi.vcxproj]
    File "C:\Users\cneumann\personal\outcome\build\quickcpplib\repo\pcpp\pcpp\preprocessor.py", line 1121, in evalexpr
      result = int(eval(expr, evalfuncts, evalvars))
    File "<string>", line 1
      not __has_include(<variant>)
                        ^
  SyntaxError: invalid syntax

  Converted expression was  not __has_include(<variant>) with evalvars = {'__has_include': 0, 'variant': 0}
ned14 commented 4 years ago

Firstly, as you mentioned, https://github.com/ned14/outcome/blob/develop/single-header/outcome.hpp is what all the package managers use, as it is completely self contained.

Secondly the python syntax errors are from pcpp, which is used to build the single header edition. It is complaining about the use of __has_include() in the preprocessor. It is benign.

Re: the lack of finding quickcpplib, that's because you need to find_package(quickcpplib) to bring it in, as the cmake error message says. So you should probably install quickcpplib like you are installing Outcome, find quickcpplib before outcome, and it should then all work great.

I guess what I'm asking boils down to: is there a mode of the build that doesn't try to be so clever about dependencies and just fails if quickcpplib is not found via the usual cmake mechanisms? Thanks!

Well, there is a balance of tradeoffs here. Would you prefer if installing Outcome also installed all its dependencies on your behalf?

cneumann commented 4 years ago

Thanks for the info and sorting out the different issues I conflated in this report - I should not write bug reports at the end of the day :(

I'm happy to first install quickcpplib to satisfy Outcome's dependency; I don't think it has to or even should do that automatically (it is not a package manager after all ;) ). I think what threw me off was that the configure phase did not simply complain about the missing dependency and failed, but instead went ahead and cloned quickcpplib, built (and installed?) it into Outcome's build directory and everything seemed to be going well. However, the installed Outcome is not actually usable (because of the missing dependency). It feels like an odd time to discover that I should have installed a library dependency after the configure/build/install steps all succeeded.

ned14 commented 4 years ago

I think what threw me off was that the configure phase did not simply complain about the missing dependency and failed, but instead went ahead and cloned quickcpplib, built (and installed?) it into Outcome's build directory and everything seemed to be going well.

You're right that that's a bit idiosyncratic, but it originally came from complaints by users due to Outcome not being "plug and play" when used as a git submodule. That's why I asked if maybe installation ought to also install dependencies, because that would then match. But, equally, if I added a script step during install which runs find_package() for the dependencies, and gives a warning if they are missing from installed, that might do?

It feels like an odd time to discover that I should have installed a library dependency after the configure/build/install steps all succeeded.

Well, configure for the consuming cmake didn't succeed. Only configure for the installing cmake did so. So everything worked as it was supposed to, in a way.

I should give a bit more context: there are a bunch of users of Outcome who have very custom cmake toolchains, and very custom install needs. Their bug reports and issue requests have driven the evolution of configure, build and install until now. Which, is to say, these are "deep cmake" folk with unusually complex needs. So the "simple cmake" folk have not received anything like as much attention, which is why your feedback would be especially useful.

I mean, most just use the single header include. Most of the remainder have very customised cmake. So, basically, people like you are very rare :) and that's why I'm interested in your opinion.

cneumann commented 4 years ago

Thanks for the background it is helpful to know where the current behavior is coming from. Let me perhaps describe what in my experience a "normal"/"simple" cmake workflow for a library that I want to add to a project looks like:

In my project:

In practice some of these steps collapse to nothing because I'd probably use vcpkg to obtain the library and dependencies and in that case not have to add extra -D arguments to cmake, but that is just convenience of using a package manager instead of doing everything by hand.

What I have seen other libraries do is add a "ThirdParty" or "External" directory to the source tree where they keep dependencies (possibly as git submodules) to ensure they are always available. Distribution packagers hate this of course, but I think as long as the build system can be convinced to use system dependencies instead (e.g. by specifying -DdependencyA_DIR=... on the cmake command line or a -DUSE_SYSTEM_DEPENDENCIES=ON switch to disable use of bundled dependencies) it is not too terrible. Not sure if that would break your power users though ;)

I'll try later today if building/install quickcpplib and then passing a -Dquickcpplib_DIR=... argument to Outcome's config step makes it pick up that instead of building it itself. EDIT: Tried it now, but unfortunately it does not recognize it and falls back to cloning quickcpplib into the Outcome build directory. CMake command line used: cmake -G "Visual Studio 16 2019" -A x64 -DCMAKE_INSTALL_PREFIX=%PREFIX%\..\install-outcome -Dquickcpplib_DIR=%PREFIX%\install-quickcpplib\lib\cmake\quickcpplib ..\outcome

If it does I guess I'd be happy if the process would either:

My preference is very much for the former - resolving dependencies is a job for a package manager in my opinion and not something a build system should have to tackle :) The latter would also cause issues for packaging in vcpkg because bundled dependencies are very much frowned upon there as well. Perhaps it could be hidden behind a switch (default ON to prevent breaking existing users?).

[1] BTW, that final step currently does not work for Outcome's vcpkg port, because the port file simply places the single header version in the shared include directory for vcpkg, but does not install outcomeConfig.cmake, so there is no exported target available. I think (but haven't tried it) this would cause an actual error for me if Outcome was the only library from vcpkg that I used for a target, because that target then would have no reason to look in the vcpkg shared include directory for any headers. Of course that is a bug in the vcpkg port of Outcome, not Outcome itself ;)

ned14 commented 4 years ago

Firstly, I've fixed the cmake bootstrap script not detecting quickcpplib_DIR.

Secondly, to test this, I did a test Outcome install into testinstall, and then wrote the following cmake:

project(test)
find_package(outcome CONFIG PATHS "../testinstall")

I ran cmake ., and cmake said:

-- Found outcome: /home/ned/windocs/boostish/outcome/build_posix_Debug/testinstall/lib/cmake/outcome/outcomeConfig.cmake
CMake Warning at CMakeLists.txt:2 (find_package):
  Found package configuration file:

    /home/ned/windocs/boostish/outcome/build_posix_Debug/testinstall/lib/cmake/outcome/outcomeConfig.cmake

  but it set outcome_FOUND to FALSE so package "outcome" is considered to be
  NOT FOUND.  Reason given by package:

  The following imported targets are referenced, but are missing:
  quickcpplib::hl

So how is this not already sufficient? It seems to me that this is working as expected?

Re an external subprojects directory, we had that before, got a lot of complaints, so it got excised a bit over a year ago in favour of the externally injectable dependencies but with auto fetch solution currently in there.

Re: your other two options, much of the problem here is a chicken-and-egg one. Outcome has a declarative build implemented by lots of cmake scripting in Quickcpplib, so it cannot stand alone. I like this, but lots of people hate this, indeed one fellow maintains a fork of Outcome where all the cmake is replaced with vanilla cmake! We could fold install of dependencies together, but I can almost guarantee you that people will complain if you install unrelated targets. The warning above seems sufficient, so I'd be interested in finding out why you weren't receiving it.

cneumann commented 4 years ago

Firstly, I've fixed the cmake bootstrap script not detecting quickcpplib_DIR

Ah, my bad, I see now that it does indeed use the quickcpplib from quickcpplib_DIR. Output from the cmake invocation for Outcome starts with:

-- quickcpplib not found, cloning git repository and installing into C:/Users/cneumann/personal/outcome/build-outcome/quickcpplib ...

Which led me to believe that it wasn't using the library from quickcpplib_DIR. But it only clones the repo; later steps detect that it is available and use the library from the location I pointed it to. Sorry for jumping to the wrong conclusion.

Secondly, to test this, I did a test Outcome install into testinstall, and then wrote the following cmake...

Interesting. I'm not getting that warning, instead find_package(outcome CONFIG) succeeds, but I get errors later on:

cmake -G "Visual Studio 16 2019" -A x64 -Doutcome_DIR=%CD%\..\install-outcome\lib\cmake\outcome ..\test
-- Selecting Windows SDK version 10.0.18362.0 to target Windows 10.0.18363.
-- The CXX compiler identification is MSVC 19.26.28806.0
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.26.28801/bin/Hostx64/x64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.26.28801/bin/Hostx64/x64/cl.exe - works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found outcome: C:/Users/cneumann/personal/outcome/install-outcome/lib/cmake/outcome/outcomeConfig.cmake
-- Configuring done
CMake Error at CMakeLists.txt:11 (add_executable):
  Target "testoutcome" links to target "quickcpplib::hl" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?

Of course, adding find_package(quickcpplib CONFIG REQUIRED) to CMakeLists.txt and the correct -Dquickcpplib_DIR on the cmake command line makes everything work and the above error is not too cryptic to figure out either. So, with the added background knowledge/history I'd say things are working as intended after all. Thanks for your patience and explanations! I'm sure you can guess I'm in the camp with the fellow who believes this setup is far too clever, but given that it automates your workflows my opinion isn't all too relevant there ;) The only sad part is that I don't think I'll be able to fix that vcpkg package for outcome now to run a regular install step and create the outcomeConfig.cmake, outcomeExports.cmake files - in the vcpkg environment CMakeLists.txt are not allowed to perform arbitrary execute_command calls, so the repo cloning step is problematic.

ned14 commented 4 years ago

I'm sure you can guess I'm in the camp with the fellow who believes this setup is far too clever, but given that it automates your workflows my opinion isn't all too relevant there ;)

Well, there are a lot of balancing factors here. Some users like to git submodule Outcome, and then add_subdirectory(). Others like to ExternalProject_Add(). Still others like to use git archive Outcome and unpack the tarball during build. And of course there are those who like to build and install, and each of the major package managers also have their own particular quirks and weirdnesses, as does RHEL vs Ubuntu vs Mac OS in terms of filesystem layouts and conventions. I try to ensure that all of the above work well, whilst also retaining a shared declarative common build system between all my projects.

The only sad part is that I don't think I'll be able to fix that vcpkg package for outcome now to run a regular install step and create the outcomeConfig.cmake, outcomeExports.cmake files - in the vcpkg environment CMakeLists.txt are not allowed to perform arbitrary execute_command calls, so the repo cloning step is problematic.

Package managers and conventional install of Outcome has always been problematic because I don't think Quickcpplib ought to publicly appear in package manager listings. I don't intend for anyone else to use it. That's what led to the single header build edition. It's one or two lines of cmake to declare an interface target, and export that, if you find just downloading a copy of the single header edition and dumping it into your project too onerous.