fix: Add AXIOM_MONO_LIBRARY

[MBkkt]

Adding AxiomUtils to make possible to configure

1. multiple velox targets and multiple axiom targets
2. single velox target and multiple axiom targets
3. multiple velox targets and single axiom target
4. single velox target and single axiom targets

This PR also contains fixes from #460

[MBkkt]

copied from velox, replaced (case sensetive) velox with axiom

[MBkkt]

I can remove it after PR will pass tests, if you think it's better without it

Suggested change

	CMAKE_FLAGS := -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DAXIOM_BUILD_TESTING=ON -DVELOX_MONO_LIBRARY=ON -DAXIOM_MONO_LIBRARY=ON
	CMAKE_FLAGS := -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DAXIOM_BUILD_TESTING=ON -DVELOX_MONO_LIBRARY=ON

or

Suggested change

	CMAKE_FLAGS := -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DAXIOM_BUILD_TESTING=ON -DVELOX_MONO_LIBRARY=ON -DAXIOM_MONO_LIBRARY=ON
	CMAKE_FLAGS := -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DAXIOM_BUILD_TESTING=ON

[mbasmanova]

@assignUser @kgpai Jacob, Krishna, would you help review this change?

@MBkkt Would you update PR description to explain what is the effect of this change and why is it beneficial? Thanks.

[MBkkt]

@mbasmanova is it ok now?

[mbasmanova]

Without this PR it's impossible to use axiom with VELOX_MONO_LIBRARY.

Why would it be helpful to use VELOX_MONO_LIBRARY? What does that do?

It's also quite bad that currently targets named randomly and some libraries are separate but some part of velox libraries. So I made naming consistent.

Would you provide an example? Would you like to introduce a naming convention for CMake targets? That would be nice, but let's document it in https://github.com/facebookexperimental/verax/blob/main/CONTRIBUTING.md

Another change, right now (in main) most test targets are configured by default and some isn't, this is fixed

Would you share an example? What do you mean "by default" and "is fixed"?

It sounds there are 3 separate changes in this PR. Would you split these into 3 separate PRs?

[MBkkt]

Why would it be helpful to use VELOX_MONO_LIBRARY? What does that do?

It's already exist in velox, it's used to produce single libvelox.a or libvelox.so instead of hundreds small .so and .a targets. It's useful to allow compiler/linker/lto to better optimize code and speedup their work.
It also decrease sizeof of build directory.
It's mainly used by users of velox and axiom.

Would you provide an example? Would you like to introduce a naming convention for CMake targets? That would be nice, but let's document it in https://github.com/facebookexperimental/verax/blob/main/CONTRIBUTING.md

velox_verax, velox_fe_*, but axiom_logical_plan, axiom_sql_parser_*, etc

I'm not sure what to write contributing, and honestly don't think it's really necessary.
New targets commonly "done" same way as previously existed targets.
So I think we just need to make current state consistent.

It sounds there are 3 separate changes in this PR. Would you split these into 3 separate PRs?

The naming is deeply related to axiom_add_library, so it cannot be separated.

But I think I can try to separate if (AXIOM_ENABLE_TESTING).

[MBkkt]

@mbasmanova I made separate PR with *_ENABLE_TESTING, I will rebase this PR if it will be merged first #376

[assignUser]

I really appreciate your efforts here but just vendoring the Velox CMake is not the right way to do this.

The wrapper functions in Velox are a workaround because the codebase was already massive and had 100+ targets. Reducing/combining those targets is a longterm goal but wasn't feasible in the short-term, hence the workaround with the mono library.

This is not the case for verax/axiom (btw what is the right name here for what? 😁), we can just create fewer more logical targets from the start and keep think organized. I am not familiar with the code base so I don't really know what that amount is, it depends on intended usage etc. (e.g. could be libverax-optimizer, libverax-runner, or something like that or just a single library if the use case would always require everything)

[assignUser]

The short-term way to get verax to work with VELOX_MONO_LIBRARY is to remove the use of velox_add_library and use add_library etc. instead (within verax/axiom).

[MBkkt]

@assignUser

This is not the case for verax/axiom (btw what is the right name here for what? 😁), we can just create fewer more logical targets from the start and keep think organized. I am not familiar with the code base so I don't really know what that amount is, it depends on intended usage etc. (e.g. could be libverax-optimizer, libverax-runner, or something like that or just a single library if the use case would always require everything)

The name is correct, as far as I know (see #211).

I don't see why do we need multiple targets for this. Maybe someone need, in such case they can build with AXIOM_MONO=OFF.
Right now already a lot of targets.

Also without this PR using verax is kind of painful, because VELOX_MONO just doesn't work.

Can we just have solution that

1. works with VELOX_MONO
2. can produce single target
3. can produce multiple target

Later we can refactor this.

[MBkkt]

The short-term way to get verax to work with VELOX_MONO_LIBRARY is to remove the use of velox_add_library and use add_library etc. instead (within verax/axiom).

It's worse solution, it produces a lot of unnecessary targets

[MBkkt]

With static build from source we don't need at all separate targets.
Because linker can drop unnecessary symbols/code itself.

At the same time, build + link time with single target (as dependency) is reduced compared to multiple dependent targets

[MBkkt]

Also with separate targets build directory in CI takes a lot of more space.

[assignUser]

It's worse solution, it produces a lot of unnecessary targets

That's why I said short-term solution. The real solution is, as I explained above, to reduce the number of targets that are created. This codebase is small enough to just make that change instead of making use of a workaround from another codebase.

Also with separate targets build directory in CI takes a lot of more space.

I don't know why you are trying to convince me that less targets are better. I know, I was the one to write velox_add_library after all ;). I am just saying that using the velox workaround is not the way to go here.

[MBkkt]

@assignUser I created this discussion for velox facebookincubator/velox#14807

[MBkkt]

I am just saying that using the velox workaround is not the way to go here.

Why? What is bad about it, except it doesn't work for PUBLIC/PRIVATE/INTERFACE (can be fixed later, same in velox)

[MBkkt]

I don't know why you are trying to convince me that less targets are better. I know, I was the one to write velox_add_library after all ;). I am just saying that using the velox workaround is not the way to go here.

So why don't you want to allow us (in serenedb) to build with single axiom target?

[MBkkt]

The real solution is, as I explained above, to reduce the number of targets that are created.

It doesn't solve my problem, it doesn't produce single axiom target

[MBkkt]

Well in theory we can make add_library( OBJECT) for separated targets, and when reference them in separated STATIC/SHARED libraries + in single(common) library.

But it's complicated plus cmake doesn't work for OBJECT with cyclic dependecies

[assignUser]

It doesn't solve my problem, it doesn't produce single axiom target

Ok? The solution for that is to create a config for axiom like we just started adding in velox which will give you one Axiom::axiom main target. (We could also add an interface target like that for use with FetchContent).

So why don't you want to allow us (in serenedb) to build with single axiom target?

I don't. I just want to do it in an idiomatic CMake way instead of vendoring tech-debt from another project. (even if I created said tech-debt ;))

Well in theory we can make add_library( OBJECT)

I agree OBJECT libraries are absolutely not the solution.

I understand your need for a single target you can link against in your downstream project and I agree, that is much better DX than having to check which of the n targets is needed (well maybe outside from high-level components like libvelox-core, libvelox-gpu, ... but that's not relevant here). But let's solve the actual problem in the right way.

[MBkkt]

@assignUser

Ok? The solution for that is to create a config for axiom like we just started adding in velox which will give you one Axiom::axiom main target.

Unfortunately it's not the good solution for serenedb.
We build as static libraries from source, each target will be created on disk (static not object library), and there's two options:

1. we create common target as interface (build and link time increased in such case)
2. we create common target as static library (everything is duplicated on disk)

I just want to do it in an idiomatic CMake

I think this is reason why it's difficult to us to agree. From my point of view there's no such thing as idiomatic cmake. cmake is just configure system to get expected binaries and files in my opinion.

And solution that you proposed unfortunately doesn't solve our problem (I can be wrong about this, so please correct me in such case).

[assignUser]

I don't have the time to look at the verax codebase in depth to propose a concert solution but rest assured that it is possible.

cmake is just configure system to get expected binaries and files in my opinion.

Yeah, and there is an idiomatic way to do things, so called 'modern cmake', you'll just have to believe the build engineer who does nothing but stare at CMake all day 🫠 😁

1. No, interface targets are virtual targets that are not actually build so the only influence they have is on configure/generate.

I can be wrong about this, so please correct me in such case

I will but won't have time for a bit to do it. I also would like some input from @mbasmanova et al

[MBkkt]

Maybe I can try to explain it in a different way.
I think this PR solves all current issues (in terms of desired binaries produced and it doesn't fail to build), maybe not the best way from cmake perspective, but still.

If someone want and can refactor it in idiomatic cmake, ofc it will be good.
But the issue even if it's possible (I'm not sure), it takes a lot of time.
Who will spend time on this?

Idiomatic cmake unfortunately doesn't get anything to our project.
So I don't have time to make cmake ideal.
I made this cmake PR only because it's needed to solve the concrete issues.
Not because I want refactor something.

And I think it makes current state better than in main branch from any perspective (even from cmake).

If it's not possible to merge this, ok, well I probably can't do anything else about it, and will maintain it as a patch

[MBkkt]

No, interface targets are virtual targets that are not actually build so the only influence they have is on configure/generate.

With interface target we will link against N small libraries, and we will build N small libraries.

Interface is just convenient way to link against these N libraries.

It's not the same as single library

[MBkkt]

I will but won't have time for a bit to do it

This the whole point, no one, at least for now, have time for this.

I just want to solve build issues and make it is a way better than in current main.

[assignUser]

I just want to solve build issues and make it is a way better than in current main.

No, you want to solve a problem that you have, without taking the overall code quality/tech debt into consideration.

Not sure why you are constrained by disk space but the majority of size when building Axiom should come from Velox (yes we can still optimize with less targets etc.), so if you just remove the use of velox_add_library in Axiom you can build against Velox with mono lib turned on and that should solve your main problem. And if you add an Axiom::axiom interface target your DX issue would also be solved. Both short term workarounds that I would accpet.

[MBkkt]

No, you want to solve a problem that you have, without taking the overall code quality/tech debt into consideration.

This isn't true, my branch cmake code quality is better than current main branch.

majority of size when building Axiom should come from Velox

It's true for now, but why do we need to choose solution that will introduce issues in future and will be worse now?
Also it's possible to build with axiom mono off.

so if you just remove the use of velox_add_library in Axiom you can build against Velox with mono lib turned on and that should solve your main problem

Yes, but it will be worse than this branch solution.

your DX issue would also be solved.

It's not the only issue that I mentioned.

Both short term workarounds that I would accpet.

This is really not collaborative approach from you.

You're really fight for writing add_library instead of axiom_add_library.
And you don't have time/it's not your part of your tasks to work on this now.

At the same time I have real build issues (I need to produce single binary) and I have solution for it, on which I spend time.
This solution also solves different issues in current main, and even makes main cmake code quality is better.
I also made it's different from what I desired -- single target for both velox and axiom (kind of current main state btw).
I made it's different because I hope it will be more appropriate to you, because I noticed that you have different compared to mine approach to cmake (you mentioned in the issue that you dislike solution where velox and axiom is single target, although it makes sense to me).

[kgpai]

Hi @MBkkt ,

Without this PR it's impossible to use axiom with VELOX_MONO_LIBRARY.

Apologies can you educate me on why VELOX_MONO_LIBRARY (assuming you have also set VELOX_BUILD_SHARED) prevents building/using axiom ?

Also looking at axiom, it seems that just linking against axiom_optimizer should help you produce your single binary.

[MBkkt]

Apologies can you educate me on why VELOX_MONO_LIBRARY (assuming you have also set VELOX_BUILD_SHARED) prevents building/using axiom ?

@kgpai Hi, ofc. There's issue related to right now for targets with axiom_ prefix used velox_add_library.

velox_add_library works only for targets with velox_ prefix.

See our patch serenedb/velox@d3d3a04 to fix this and my issue #210

[kgpai]

Hi @MBkkt ,

1. I believe Verax should move off velox_add_library and just use add_library. The VELOX_MONO build issues you are seeing might be due to this.
2. Velox will make a change to add a warning if it detects velox_add_library is being used outside velox
3. Axiom can create an all encompassing interface target that can be linked against externally ?

I also confess I dont fully understand the multiple target problem and why that is an issue, but I am keen to understand the build issues you are seeing so we can fix it and other projects can use standard cmake and make things work out of the box.

cc: @mbasmanova @assignUser

[MBkkt]

Hi @kgpai

I believe Verax should move off velox_add_library and just use add_library. The VELOX_MONO build issues you are seeing might be due to this

I know the reason, I can fix it in a different ways. The different ways have different tradeoffs.
I don't ask why is it fail, I suggested fix.

Fix that just replaces everything with add_library/etc, is bad IMO.
Btw it was such PR from me too, but I decided to close it, because I dislike the idea.

Velox will make a change to add a warning if it detects velox_add_library is being used outside velox

Sounds reasonable to me, its not difficult to implement.

Axiom can create an all encompassing interface target that can be linked against externally ?

This is different from PR approach that I discussed above with assignUser.
The main downside of this approach, it will be physically still different libraries (different .a or .so)

I also confess I dont fully understand the multiple target problem and why that is an issue, but I am keen to understand the build issues you are seeing so we can fix it and other projects can use standard cmake and make things work out of the box.

IMO in ideal world any C++ library can be produced with single target and library.

VeloxUtils approach allows this for velox and I think we need to do same for AxiomUtils.
Even if such pattern isn't cmake standard practice.

[kgpai]

Hi @MBkkt ,

Firstly thank you for patience and understanding. There are still many gaps in my understanding here - Would it be possible for you to reach out to me on slack to help me out ? My slack id is ([email protected]) .

[assignUser]

@kgpai please also see my comment in #387