Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[donut merge] Conan example. #71

Closed
wants to merge 1 commit into from
Closed

[donut merge] Conan example. #71

wants to merge 1 commit into from

Conversation

p-groarke
Copy link
Contributor

Hey, I was playing around with the lib to get a better lay of the land and I couldn't help myself :)

Here's an example how you can use conan to quite easily install your deps. All you need is conan installed and the rest is automagic.

Concearning #55 , I hadn't realized you took care to hide pugi behind your pimpl abi boundary. As far as I know, that will prevent dependency hell and you can close that issue.

You can use the conanfile.txt to install catch/gtest as well.

Here's an unrelated question, what is DevScripts? Is the library generated through ruby?

Good day

@@ -0,0 +1,12 @@
[requires]
pugixml/1.10@bincrafters/stable
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you would add catch or other libs here.

Copy link
Owner

@webern webern Mar 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I haven't been clear enough about this, and I've added an issue, #72, to address this directly in the documentation.

Zero-dependency Build is a tennent of this library. As such, the following statements should always remain true:

  • find_package should not be used in any CMake files.
  • A package manager should not be required to build this library.
  • The mx library should never depend on the existence on dynamic linking to third-party libraries outside of its own code repository.
  • The amount of third-party code statically baked-in to the library should be minimal.

In support of this tennant, mx has as few dependencies on third-party code as possible. Currently the only dependency is pugixml, which consists of about three code files. pugixml is wrapped in another 'third-pary' library, my own ezxml, which again introduces no other dependencies beyond pugixml. These are checked-in to the mx repository so that the user of mx need not obtain them elsewhere.

Currently there there are two issues to fix that are counter to the above goals:

  1. We use find_package for pthreads. This is only needed during test runs, so this should be moved such that it only occurs during building of the test code. (Move pthreads dependency to test only. #75)
  2. When you added the install statements you also 'installed' ezxml. This should be removed. This is a transparent 'baked-in' dependency and should not be installed by mx. (Remove install_package for ezxml #76)

I understand your concern about dependency hell, but I promise you that mx does not participate in it. To that end, mx relies on almost nothing. The consumer of mx need not concern themselves with what its dependencies are. Instead they are getting a zero-dependency, statically complete library.

All this being said, publishing mx on Conan sounds like a great idea. So I've added an issue for that, #73. Just be aware that publishing it on Conan does not mean that we will use find_package or use Conan or any other package manager to find any of mx's dependencies outside of its own code repository. They will remain 'baked-in' to both the static binary and the code repository.

Perhaps when we publish it through Conan you can rely on it through that package manager and forget about the pseudo-dependency that it has on 3 code files of third party code. :)

Additionally, I make the following promise: If mx ever conflicts with any other dependencies in anyone's build, I will fix it within a few hours of receiving the issue. :) I am absolutely confident on this matter. In fact, as it stands now it is almost (perhaps completely) impossible and I could fix it in seconds by renaming a namespace.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding your other question, yes I generated most of mx::core with Ruby. It was a long time ago but those scripts are a fun historical artifact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I simply wanted to demonstrate how conan works. Consuming mx through conan would be great!

I think you have some misconceptions on how the libs are linking and my previous install scripts.

You link to ezxml publicly with target_link_libraries(${PROJECT_NAME} PUBLIC ezxml). So consumers of mx need to be able to link with ezxml transiently. That is why I had to install it. To remove that requirement, simply change PUBLIC to PRIVATE in the link command and fix whatever errors pop up.

I did not and would not change that. That's how you wrote your cmake file.

Using conan for gtest or catch doesn't change anything to your exposed libraries. Consumers of your lib don't care about the tests. You simply do

if(MX_BUILD_TESTS)
    find_package(catch REQUIRED)
    target_link_libraries(MxTest ${PROJECT_NAME} catch::catch)
# ...

for example. You are not leaking a library.

Cheers

Copy link
Owner

@webern webern Mar 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a mistake that should be fixed target_link_libraries(${PROJECT_NAME} PUBLIC ezxml)

It seems like this is the source of the disconnect between our conceptions of the build.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find_package(catch REQUIRED) does not make sense to me, even the author says that you can simply drop into your repository link

Catch2 is header only. All you need to do is drop the file [...] directly into your project tree itself! This is a particularly good option for other Open-Source projects that want to use Catch for their test suite.

Why would we want to add the step of obtaining the code somewhere out of band over what we have now? Currently there are zero steps involved for the developer who wants to run the mx tests. I'm having a hard time understanding the advantage of using find_package for anything in mx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About catch, absolutely. I am not commenting on what you should do. It is just an option. For example, a lot of people use catch exactly for that reason. However with conan, that need not be your main criteria for selecting libraries, aka "must be a header only library because I don't want to fight with my darned linker again" XD

Just to make things clear, I am NOT recommending you do anything. I love working with mx. This PR was just for fun and learning. I couldn't care less what you do as long as I can consume mx without problem.

Now back to the ezxml leaking. I would've loved knowing about your library philosophy when I opened that pr ;) I didn't add the ezxml install step because I wanted to, I really didn't want to. I am super lazy.

The build didn't work without it because ezxml declares it a public dependency. Without you knowing it, ezxml has always been a transitive dependency of mx and I guess nobody ever reported it. However, you built ezxml completely ready to be hidden and have been very diligent not to expose any of its headers.

So, I will open another PR with a potential fix. However, please try it out. It will make editing ezxml a bit more of a pain since it won't show up in your IDE like before. add_subdirectory is a shit show when it comes to install targets. There are tons of tickets and bug reports about. So I'll use something else.

In the future, please just ask if you aren't happy about the cmake stuff.

Copy link
Owner

@webern webern Mar 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for snapping a little bit. I appreciate the feedback. I would be happy to handle ezxml without add_subdirectory at all, just add its files inline in the top-level cmake file and delete the ezxml cmake file. It's pretty easy to backmerge any useful changes to ezxml that occur in the mx repo.

@p-groarke p-groarke closed this Mar 28, 2020
@p-groarke p-groarke deleted the pugi_exampl branch May 3, 2020 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants