Make boost optional dependency

[ArtemSarmini]

Steps to achieve it:

• There's only 1 place which always depends on boost, computation of return type for zug::sequence. Could it be changed?
• Document usage of ZUG_VARIANT_{STD,BOOST} and ZUG_ENABLE_BOOST, add cmake option to switch variant implementation.
• Integrate boost into cmake (to give ability to use boost from non-standard path easily).

[arximboldi]

Yes, that's a good point. I agree both boost::optional and boost::variant should be optional (lol) and use a common and simple mechanism to turn them on/off. Most new projects I guess will be on C++17 I'd hope and their std counterparts should be used instead automatically.

[yesudeep]

I've done this for Bazel in my fix-bazel-build branch, which adds rules for testing as well.
There are other places where the library depends on boost too, I think. I've added
2 definitions of the library one with boost and one without boost to accommodate for both types
of user. The library user only needs to depend on either @zug//:zug or @zug//:zug_with_boost and it pulls the appropriate dependencies in recursively.

However, the code may need some tweaking as well as I'm running into a few problems:

1. I've switched to using the #include "..." form instead of "#include <...> because it allows both locally vendoring zug in my source tree using Bazel and installation.
2. I tried defining the ZUG_VARIANT_BOOST macro at the command line but ran into macro redefinitions:

❯ bazel test ...
INFO: Build option --cxxopt has changed, discarding analysis cache.
INFO: Analyzed 14 targets (27 packages loaded, 4646 targets configured).
INFO: Found 4 targets and 10 test targets...
INFO: From Compiling test/into.cpp:
In file included from test/into.cpp:11:
In file included from ./zug/transducer/filter.hpp:12:
./zug/skip.hpp:16:9: warning: 'ZUG_VARIANT_BOOST' macro redefined [-Wmacro-redefined]
#define ZUG_VARIANT_BOOST 1
        ^
<command line>:4:9: note: previous definition is here
#define ZUG_VARIANT_BOOST 0
        ^
1 warning generated.
INFO: From Compiling test/into_vector.cpp:
In file included from test/into_vector.cpp:11:
In file included from ./zug/transducer/filter.hpp:12:
./zug/skip.hpp:16:9: warning: 'ZUG_VARIANT_BOOST' macro redefined [-Wmacro-redefined]
#define ZUG_VARIANT_BOOST 1
        ^
<command line>:4:9: note: previous definition is here
#define ZUG_VARIANT_BOOST 0
        ^
1 warning generated.
INFO: Elapsed time: 13.593s, Critical Path: 11.55s
INFO: 40 processes: 40 darwin-sandbox.
INFO: Build completed successfully, 51 total actions
//test:compose_test                                                      PASSED in 0.3s
//test:into_test                                                         PASSED in 0.2s
//test:into_vector_test                                                  PASSED in 0.2s
//test:meta_test                                                         PASSED in 0.2s
//test:reduce_test                                                       PASSED in 0.2s
//test:reductor_test                                                     PASSED in 0.2s
//test:run_test                                                          PASSED in 0.2s
//test:sequence_test                                                     PASSED in 0.2s
//test:state_traits_test                                                 PASSED in 0.1s
//test:transduce_test                                                    PASSED in 0.3s

[yesudeep]

Some tests don't compile with GCC 10.0 on Linux without Boost, but do using clang 10.0.0 on Mac OS X 10.15.6.

I'm wondering whether #define ZUG_VARIANT_BOOST 1 in skip.hpp is a workaround for a potential GCC bug?

[arximboldi]

@yesudeep I think you have to remove the ZUG_VARIANT_BOOST in skip.hpp. That code should defo be improved, by trying to detect C++17 directly, and then relying on configurable macros from the outside.

[yesudeep]

@arximboldi Agree. I've currently been trying to make the Bazel build work cross-platform while avoiding as many code changes as I can. I'm not sure but would you prefer to remove the dependency on Boost over the longer term? I've added an alias that maps the :zug target to :zug_with_boost for now so that library users depending on @zug//:zug can continue using Zug without concerning themselves with whether they are using Boost or otherwise. If at a later date the library no longer uses boost, they wouldn't notice as it involves no code changes for them, except their build times may go down.

Also, would you be willing to accept PRs to add more comprehensive Bazel integration alongside CMake since you already have some support? It would make life easier for C++ programmers. The following is a lot easier than fighting with the build system/environment :) :

cc_binary(
    name = "myapp",
    srcs = ["myapp.cpp"],
    deps = [
       "@zug",  # Causes Bazel to fetch, include, compile, and link along with its dependencies.
    ],
)

Do consider checking out this branch and taking it for a spin.

Aside: You may also want to consider making C++ 17 the minimum requirement. People should be using it by now.

Use --sandbox_debug to see verbose messages from the sandbox
test/reduce.cpp:39:16: error: constexpr variable cannot have non-literal type 'const (anonymous namespace)::(lambda at test/reduce.cpp:39:22)'
constexpr auto foo = [](auto step) {
               ^
test/reduce.cpp:39:22: note: lambda closure types are non-literal types before C++17
constexpr auto foo = [](auto step) {
                     ^
1 error generated.

[arximboldi]

Also, would you be willing to accept PRs to add more comprehensive Bazel integration alongside CMake since you already have some support?

Yes! I do believe that boost should be fully optional for this library, and better Bazel support would be highly appreciated. I am no Bazel expert though, but I'd be happy to review and integrate whatever you come up with.

Aside: You may also want to consider making C++ 17 the minimum requirement. People should be using it by now.

Would rather not yet, sadly in commercial projects upgrading the stantard can often be very difficult, and the scope of this library is so limited that I see no reason to force it.

[arximboldi]

I've taken a look into your other branch @yesudeep and it is looking good in general, but it seems to me that there are unnecessary changes. In particular, you changed all the includes from <> to "", something that I see absolutely no need to and I prefer the former style (works sometimes better when the library is installed in the system).

[yesudeep]

Hi @arximboldi I had originally used quotes instead of
angled brackets to fix build errors. I've used a
workaround to keep <angled> includes around,
but it isn't without side effects. Here's a discussion
with the Bazel folks about this.

Do take this updated branch for a spin.
I'll polish it up before sending a PR.

[arximboldi]

So the solution is to have an include directory that, in this case, is a symlink to the actual code? I would even consider actually moving the code there... I'll think about it.

[BlueSolei]

I created PR #30 to solve the dependency in boost at least when you build with C++17