fpagliughi / sockpp

Modern C++ socket library.
BSD 3-Clause "New" or "Revised" License
782 stars 126 forks source link

Refactoring the CMakeFiles.txt files #46

Closed arsdever closed 2 years ago

arsdever commented 4 years ago

Fixes #45

fpagliughi commented 4 years ago

Thanks for the PR! I've been meaning to get to the export config stuff for a while now, but just haven't found the time. This looks awesome/simpler if it works across all the different targets.

What platforms did you test this on?

In Windows, if you turn on both static and shared builds, how does it deal with naming the static lib vs the import lib for the DLL, since they both use the same ".lib" extension?

arsdever commented 4 years ago

What platforms did you test this on?

I tested only on Windows 10 VS 2019.

Will test also on Ubuntu 18.04 terminal from WSL.

In Windows, if you turn on both static and shared builds, how does it deal with naming the static lib vs the import lib for the DLL, since they both use the same ".lib" extension?

I didn't think about this scenario. BTW is the option useful to build for both shared and static targets at the same time?

Does this make sense?

fpagliughi commented 4 years ago

The way it is currently implemented, it can build both shared and static, if you ask for both. That's why it specifically named the static library differently with the "-static" appended to the base name.

Note that this "-static" name addition is only required on Windows. On *nix systems, it can build the static lib as ".a", and the shared lib as ".so"

fpagliughi commented 4 years ago

Note, in the original CMake file, it specifically renamed the output for the static build so that it would have the same base name as the shared library (no "-static"):

# Let the archive use the same name as the shared library
if(UNIX)
    set_target_properties(${SOCKPP_STATIC} PROPERTIES OUTPUT_NAME ${SOCKPP})
endif()

I'm not fixed on doing things one way or another, but I would want to be careful about breaking people's builds or removing features if we didn't have to. Most people would know if they want one or the other shared/static, but if you're doing a generic build for a distribution, you probably want to build both versions and let individual users/projects decide which to link to.

arsdever commented 4 years ago

Yes, I understand, but it doesn't answer my question. What is the use-case to build both (shared and static) libraries at the same time?

For the second part of your response, I would say that instead of removing the option, we can declare it as deprecated and add a KB article to notify people not to use both targets together. Another approach is, that we can specify a priority and ensure that only a single option is chosen. There are a lot of approaches, but first, we should understand the answer to the question above. Right?

In addition, want to mention, that I didn't see an option to set whether I want to build a static or a shared library in other libraries (at least in json and spdlog)

fpagliughi commented 4 years ago

First, I agree completely, that what you want to add is really important. I want to get it in there, too.

What is the use-case to build both (shared and static) libraries at the same time?

I assume the most common reason would be to have both versions on a shared computer where different users or different applications have need of one version of the library or the other. I have multiple applications using this library, and though most use the shared library, some (by customer dictate) link to the static lib. So it's convenient to have both installed.

But if you're asking whether it would just be easier to have a user run the build twice to get the shared library one time and the static library the next, then I'm not sure. Can that be done without the install overwriting anything important?

But the main point I was making is that it currently does both, which is why I did it this way rather than just using BUILD_SHARED_LIBS. I'm not sure what people currently need, one way or another, but I'd hate to take something away if people already rely on it.

I think what you want to add is really important. But it would be cool to figure out if there is a way to add it without taking away something else.

arsdever commented 4 years ago

You completely answered my question. Then, let me think about solution. I will update PR as soon as I find it out.

fpagliughi commented 4 years ago

Yes, apologies that I'm a bit of a novice with CMake, so I don't have a ready solution either, but hopefully we can figure out something easy enough. And I appreciate your work on this.

arsdever commented 4 years ago

Here I'm listing the versions I think are applicable (this comment will be modified and updated later)

  1. Providing 2 targets: sockpp-shared and sockpp-static for shared and static libraries respectively. If the user wants to link to shared, he/she must cmake the project according to the needs and then use find_package(sockpp-<libtype>) as required.
arsdever commented 4 years ago

BTW I'm also new to CMake and find this very important to also learn CMake ;) This is why any comment to my PR is very useful and helpful (not to make mistakes)

arsdever commented 4 years ago

@fpagliughi Can you please explain why it prefers shared lib to take place into testing? https://github.com/fpagliughi/sockpp/blob/93855d54e78ea2684abe0f75caf4dbdf98bffcab/CMakeLists.txt#L176-L180

fpagliughi commented 4 years ago

It prefers the shared library for testing because I was primarily using the shared library for my personal deploys! Plus shared libraries tend to have a few more quirks than just linking in a static library.

But it's not something I fell very strongly about one way or another.

arsdever commented 4 years ago

I'm quite done with the solution I've mentioned above. Just to clarify. Is it problematic that I change the shared library name to sockpp-shared instead of sockpp? It isn't critical for me but makes sense to the customer to explicitly mention in their project whether they want a shared or static library. How do you think?

arsdever commented 4 years ago

Actually, after the last comment, I lost all the changes I made and I'm happy about that because I was in the wrong way. 😄 Currently, I wonder why we install the examples? Do we need them to be installed? I'm removing their install action and preparing the new PR. If it is needed to leave them please inform me.

fpagliughi commented 4 years ago

I would think to keep the shared library name as just "sockpp". Also remember that both libraries install as "sockpp" on non-Windows targets. I certainly don't want the weirdness of Windows to affect *nix systems, in any way, if possible. :-)

As for the examples, yes they should be installed if the user requests them to be built! (But they're not built, by default).

arsdever commented 4 years ago

As for the examples, yes they should be installed if the user requests them to be built! (But they're not built, by default).

I mean the building is, of course, a must, if requested. But what about installing? Why is it needed? Isn't it needed to install things only when you're going to include them in your project? Or I have some misunderstanding here.

fpagliughi commented 4 years ago

Well remember that you have a specific view of the library! You're thinking of it in terms of just a subproject to be included in a build of your own application. In that case, you would not want or ask for the examples to be built. And they would not be installed.

But if, for whatever reason, you want to build and install the project independently, to be shared amongst many applications, and you ask for the examples to be built, you would expect them to also be installed. In most cases, the build directory is temporary, and everything in it will be erased after the install completes. So, whatever you asked to be built, you would expect to be installed. Otherwise, why would you ask for it to be built?

arsdever commented 4 years ago

Any comments on the PR update?

fpagliughi commented 4 years ago

Apologies that I'm really busy on another open source project that I need to get out in the next week or so, then I can take a closer look at this.

arsdever commented 4 years ago

No worries. By the time I continue to experience with the lib. I will make some touches if needed before merging.

fpagliughi commented 2 years ago

Hey @arsdever! Sorry it took me two years to get to this, but I'm finally back to doing some C++ work and getting a little more comfortable with CMake. I had to do some manual merging since the fork is gone, but I think I got everything. I put it all in a cmake-refactor to play with it a little. So far, it looks really good.

Thanks again.