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

Add au recipe #25447

Merged
merged 6 commits into from
Oct 15, 2024
Merged

Add au recipe #25447

merged 6 commits into from
Oct 15, 2024

Conversation

ericriff
Copy link
Contributor

Summary

Changes to recipe: au/[0.3.5]

Motivation

Add recipe for https://github.com/aurora-opensource/au

This is an incomplete recipe, it only exposes the main Au::au target and not the io component (which is used for the << operator).

I'm not sure why it is needed since it works anyway? Their CMake is a bit weird.


@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@chiphogg
Copy link

I'm not sure why it is needed since it works anyway? Their CMake is a bit weird.

Hi! I'm commenting here so that I can stay in the loop on this PR. (And also --- thanks so much for adding Au to conan! Hugely appreciated.)

A couple of questions:

  1. You're not sure why what is needed, since what else works anyway? I wasn't able to figure this out from reading the rest of the PR summary.
  2. What about our CMake is a bit weird?

For question (2), note that I'm not at all offended or anything --- this was my first time using or writing CMake, so I definitely expected it to look weird, compared to what experienced people would write! Instead, I'm asking because this seems like an awesome learning opportunity for me. I did buy the CMake book and try to follow best practices using the latest CMake version, but I'm also coming from a bazel-first mindset, so I'm sure I have many blind spots.

Thanks again 🙂

@ericriff
Copy link
Contributor Author

ericriff commented Sep 30, 2024

You're not sure why what is needed, since what else works anyway? I wasn't able to figure this out from reading the rest of the PR summary.

If you look at the test package, I'm including the io header and std::cout-ing something constructed with meters(), without "linking" in Au::io. Yet it works anyways.
Just in case you're not familiar with conan, the usual workflow here is to remove all the library-provided CMake files from the packages and use the conan-generated ones. In this case, I'm removing the AuConfig.cmake and friends, so the targets Au normally provides are ignored. This recipe on its current form only provides the Au::au target.

What about our CMake is a bit weird?
For question (2), note that I'm not at all offended or anything --- this was my first time using or writing CMake, so I definitely expected it to look weird, compared to what experienced people would write! Instead, I'm asking because this seems like an awesome learning opportunity for me. I did buy the CMake book and try to follow best practices using the latest CMake version, but I'm also coming from a bazel-first mindset, so I'm sure I have many blind spots.

Let me start with an apology, I did not meant to criticize your code. English is not my first language and I may have said something that came out disrespectful, that was not my intention at all.
What I find odd is that

  1. GTest gets "leaked" to the consumer of Au. It should 100% be an internal thing, the consumer of Au shouldn't have to have it installed to be able to use this lib. This helps minimize the footprint of the library and simplifies packaging.
  2. The CMake macro that declrares your headers only targets automatically defines tests, which makes the decoupling of the packages library and testing a bit harder. A common practice is to do something like this
add_library(myCoolLibrary)

target_sources(..)
target_link_libraries(...)

if(BUILD_TESTING)
   add_subdirectiory(tests)
endif()

Then you completely ignore the tests with a single flag.

@chiphogg
Copy link

chiphogg commented Oct 1, 2024

No apology necessary. 🙂 The only reason I mentioned "I'm not offended" was because of what I was saying in that message: I thought I could easily be mistaken for getting defensive, since tone doesn't translate well on the internet.

I think I'm starting to see the outlines of the changes we'll want to end up making. Probably something like the following:

  • Consolidate the library targets into two: one "main" one, and one for testing which depends on GTest
  • Rework the header-only library utility to separate out test creation, since library targets won't be 1:1 with test targets in CMake anymore
  • Create appropriate CMake options, which should make it easy to both (a) avoid building tests, and (b) avoid bringing in the GTest dependency (which are separate issues)

I'll need to go back and remember all the different use cases I tested when we first added CMake support (system install, FetchContent, ...), and make sure all those still work. And we'll need to test the new use cases you're providing. But I think we can make this work.

I'm assuming that once Au is in shape, we wouldn't need the patch file that's currently in this present PR --- does that sound right to you?

@AbrilRBS AbrilRBS self-assigned this Oct 1, 2024
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Hi @ericriff thanks a lot for taking the time to create the recipe for Au, we really appreciate it (And more so me personally, I've wanted to add this for a bit now (see aurora-opensource/au#257) but hadn't found the time to do so)

This looks good to me as a first approach, but I'll need to circle back to this after rechecking @chiphogg comment in aurora-opensource/au#297 (comment) :)

@ericriff
Copy link
Contributor Author

ericriff commented Oct 1, 2024

I'm assuming that once Au is in shape, we wouldn't need the patch file that's currently in this present PR --- does that sound right to you?

Exactly. Once Au doesn't publicly depend on GTest or any other internal tool, this patch will go away.

@chiphogg
Copy link

chiphogg commented Oct 3, 2024

Just in case you're not familiar with conan, the usual workflow here is to remove all the library-provided CMake files from the packages and use the conan-generated ones. In this case, I'm removing the AuConfig.cmake and friends, so the targets Au normally provides are ignored. This recipe on its current form only provides the Au::au target.

This part was really intriguing and surprising to me. So if a library has their own CMakeLists.txt files, and then they want to add conan support, conan will ignore those files and generate its own? Is there a good reference where I can read about and understand this?

Also, I'm working on the Au-side changes, but it's a pretty busy and exciting time at work right now, so I don't have as much bandwidth right now as I often do at other times. I'll do my best to keep the ball moving forward, though.

@ericriff
Copy link
Contributor Author

ericriff commented Oct 3, 2024

Just in case you're not familiar with conan, the usual workflow here is to remove all the library-provided CMake files from the packages and use the conan-generated ones. In this case, I'm removing the AuConfig.cmake and friends, so the targets Au normally provides are ignored. This recipe on its current form only provides the Au::au target.

This part was really intriguing and surprising to me. So if a library has their own CMakeLists.txt files, and then they want to add conan support, conan will ignore those files and generate its own? Is there a good reference where I can read about and understand this?

Not exactly. The CMake files conan replaces are the ones used to find the libraries once installed. In this case, AuConfig.cmake and AuVersion.cmake.
It also removes the find Modules if they're provided (in this case it would be something like FindAu.cmake) and the package config files (Au.pc).

@AbrilRBS
Copy link
Member

AbrilRBS commented Oct 3, 2024

Note that what @ericriff is saying is refering to the Conan Center Index context. Users adding their own support for Conan in their libraries are free to use their own config files without issue :)

@chiphogg
Copy link

chiphogg commented Oct 3, 2024

Neat! So is there a future world where it makes sense to include direct support for conan inside of Au? Should we consider moving in that direction eventually?

Copy link

@chiphogg chiphogg left a comment

Choose a reason for hiding this comment

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

I had a general question w.r.t. strategy for this PR.

Right now, this PR patches the stock 0.3.5 version of Au. Simultaneously, we're working on updating Au's CMake upstream, to be more friendly to conan (and vcpkg, I think). But those changes obviously won't affect 0.3.5, which is set in stone at this point.

Does that imply that (1) we should plan to land this present PR, in close to its present form, regardless of what's going on in the Au repo? Does it mean (2) Au should try to get a 0.3.6 release out soon, as the "first conan release"? Or does it mean that (3) we should use a specific (near-future) commit from HEAD as the release here?

I'm open to suggestion here. I assume (3) is a no-go. I'm guessing we want to do (1); but as for (2), I'd be open to consider cutting a release sooner than I had been planning.

recipes/au/all/test_package/test_package.cpp Outdated Show resolved Hide resolved
@AbrilRBS
Copy link
Member

AbrilRBS commented Oct 5, 2024

@chiphogg

Does that imply that (1) we should plan to land this present PR, in close to its present form, regardless of what's going on in the Au repo? Does it mean (2) Au should try to get a 0.3.6 release out soon, as the "first conan release"? Or does it mean that (3) we should use a specific (near-future) commit from HEAD as the release here?

We can go for a combination of both. We can release 0.3.5 as-is, and you can take the necessary time for 0.3.6. we'll then update the recipe without issue to handle the new version, so you can always release at your own cadance. How does that sound?

@chiphogg
Copy link

chiphogg commented Oct 6, 2024

@chiphogg

Does that imply that (1) we should plan to land this present PR, in close to its present form, regardless of what's going on in the Au repo? Does it mean (2) Au should try to get a 0.3.6 release out soon, as the "first conan release"? Or does it mean that (3) we should use a specific (near-future) commit from HEAD as the release here?

We can go for a combination of both. We can release 0.3.5 as-is, and you can take the necessary time for 0.3.6. we'll then update the recipe without issue to handle the new version, so you can always release at your own cadance. How does that sound?

Makes sense. Let's do that!

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

Warning

Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement.

All green in build 8 (3bc2095b5ccffd2f8510f67f9e448e6346db5efb):

  • au/0.3.5:
    Built 5 packages out of 11 (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 8 (3bc2095b5ccffd2f8510f67f9e448e6346db5efb):

  • au/0.3.5:
    Built 4 packages out of 5 (All logs)

@AbrilRBS
Copy link
Member

AbrilRBS commented Oct 9, 2024

Some background on the holdup for this PR until I get a chance to discuss it with the team:

au::testing seems to be a target that requires normally on gtest, but this is a bit problematic in Conan, as this could create potential conflicts with users using gtest for testing as a test_requires, which would be suprising.

@chiphogg can you give me some more insight into this target? What's is intended usage? I see that it declares some matchers, but it would be nice to get a bit more insight

@chiphogg
Copy link

chiphogg commented Oct 9, 2024

Yep, that's right: au::testing is a library target that depends on gtest, despite not being a test. The point is to define some testing helpers to make it easier to write tests with Au quantities. For example, the IsNear(...) matcher is an effective workaround for a very longstanding defect in the EXPECT_NEAR macro from gtest.

au::testing is a "leaf" target: nothing in the library depends on it. For conan, we could simply omit it. The only downside would be that users who do use gtest wouldn't have access to it. If we could figure out a way for them to "opt in", that'd be great, but the main thing is to get the core library onto conan IMO.

Also, the current version on main has been reworked so that Au::au and Au::testing are the only two public facing library targets, FWIW.

@AbrilRBS
Copy link
Member

So just to clarify, is this target always used as part of gtest testing?

@chiphogg
Copy link

So just to clarify, is this target always used as part of gtest testing?

Yes. People who use gtest would find this target helpful in writing their tests. But people who use other testing libraries wouldn't get any use out of it.

@AbrilRBS
Copy link
Member

Great! So, what we're doing is:

  • Let's merge as-is. People that use gtest will already have the necessary targets so that using au::testing is not an issue
  • Once the library has a compilation step too, we can come back and add a gtest dependency with version ranges as to minimize the risk of version conflicts

Thanks a lot for your insight and patience while we got the PR reviewed @chiphogg, and thanks @ericriff for actually taking the time to create it, we really appreciate it :)

Copy link
Contributor

@ErniGH ErniGH left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 5541c3a into conan-io:master Oct 15, 2024
9 checks passed
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.

6 participants