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.

Sign up for GitHub

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

Jump to bottom

abseil: add components + cache cmake #2928

Merged
merged 5 commits into from
Sep 29, 2020
Merged

abseil: add components + cache cmake #2928

merged 5 commits into from
Sep 29, 2020

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Sep 15, 2020 •
edited
Loading

Specify library name and version: abseil/all

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

closes #2924, closes #1697

Suggestions are welcome 😄

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 15, 2020

Another option would be to:

  • parse abseil-targets.cmake in package method (before deleting it obviously)
  • generate the json file introducted in this PR
  • install this json file (or whatever format) in package folder

@conan-center-bot
Copy link
Collaborator

Some configurations of 'abseil/20200205' failed in build 1 (4c9aa9775208d6e3575394b1100cd79aeaa1c414):

@SpaceIm SpaceIm mentioned this pull request Sep 15, 2020
Closed
@conan-center-bot
Copy link
Collaborator

Some configurations of 'abseil/20200205' failed in build 2 (4c9aa9775208d6e3575394b1100cd79aeaa1c414):

@conan-center-bot
Copy link
Collaborator

Some configurations of 'abseil/20200225.2' failed in build 3 (767565b4baaed93f6732555c59aa1228ec96b58d):

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 15, 2020 •
edited
Loading

I can wait conan 1.29.0 in CCI, or maybe using cmake_find_package in test_package, which is less buggy in 1.28.1 than cmake_find_package_multi I believe.

@SpaceIm
abseil: use cmake_find_package in test_package
c84dbe7 
cmake_find_package_multi doesn't take into account frameworks
in conan 1.28.1.
@Croydon
Copy link
Contributor

Croydon commented Sep 15, 2020

Did you generate the JSON files? (I really hope so 🙈)
If yes, how?

prince-chrismc
prince-chrismc reviewed Sep 15, 2020
View reviewed changes
recipes/abseil/all/test_package/CMakeLists.txt
include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
conan_basic_setup()
conan_basic_setup(TARGETS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
conan_basic_setup(TARGETS)
conan_basic_setup()

prince-chrismc
prince-chrismc reviewed Sep 15, 2020
View reviewed changes
recipes/abseil/all/conanfile.py
Comment on lines +56 to +60
tools.replace_in_file(
os.path.join(self._source_subfolder, "CMakeLists.txt"),
"project(absl CXX)", """project(absl CXX)
include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
conan_basic_setup()""")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be avoided with a top level CMakeLists.txt wrapper?

@prince-chrismc
Copy link
Contributor

Another option would be to:

  • parse abseil-targets.cmake in package method (before deleting it obviously)
  • generate the json file introducted in this PR
  • install this json file (or whatever format) in package folder

I think keeping the parser with the recipe would make maintenance easier!

@conan-center-bot
Copy link
Collaborator

All green in build 4 (c84dbe773c61c2d100e76b542d461be3b633cdf1)! 😊

@SSE4 SSE4 requested review from danimtb and uilianries September 17, 2020 17:43
@Croydon
Copy link
Contributor

Croydon commented Sep 18, 2020

Regarding the new auto-merge feature:

If I would "request changes" now, because I think this should not get merged until we have talked further about the generating of the JSON files - would the bot care about this? Or does the bot exclusively look at the approvals?

Croydon
Croydon suggested changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@Croydon Croydon left a comment

Choose a reason for hiding this comment

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

How to generate the JSON files?

@danimtb
Copy link
Member

danimtb commented Sep 18, 2020

@Croydon we are still kind of testing it (now in production), as the automation is configurable. The usage is very basic right now and it counts the reviews from the team and the community and only "blocks" the merge if the latest commit has a red review from any of the team members. We are still doing some changes until we enable it with the proper configuration. The details will be documented as requested in #2857

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 18, 2020 •
edited
Loading

@Croydon @prince-chrismc
It was partially generated with a small script taking absl-target.cmake file as input, but I had to manually fix some parts in generated file (like system libs) and I don't know if input file is consistent across CMake version.

I know that current recipe is far from ideal, but at least it brings all abseil imported targets in cmake_find_package and cmake_find_package_multi generators. When I have time, I'll try to really automatize creation of this json file at "package" time.

@prince-chrismc
Copy link
Contributor

I think I would prefer if you checked it in, god forbid someone else needs it at least we have a starting point.
If you do not commit the script, adding a new version would be much more complicated for anyone else.

As long as ur script says "FYI i do not work and you'll need to manually fix some paths" I think we will be miles of head of where we are today.

does that sound reasonable?

@mathbunnyru
Copy link
Contributor

mathbunnyru commented Sep 29, 2020 •
edited
Loading

lts_2020_09_23 was released (https://github.com/abseil/abseil-cpp/releases/tag/20200923).

May I ask you to generate the JSON file for the new version as well?
Totally fine, if it will be done in another PR.

@uilianries
Copy link
Member

lts_2020_09_23 was released (abseil/abseil-cpp@20200923 (release)).

May I ask you to generate the JSON file for the new version as well?
Totally fine, if it will be done in another PR.

Another PR please, this one is related to components only.

@conan-center-bot conan-center-bot merged commit 1666d9f into conan-io:master Sep 29, 2020
@SpaceIm SpaceIm deleted the fix/abseil-components branch September 29, 2020 20:58
@Croydon
Copy link
Contributor

Croydon commented Sep 30, 2020

@SpaceIm Please provide your script somewhere, even if it is just in a GitHub Gist or whatever. Otherwise we pretty much have a bus factor of 1 for adding new abseil versions and we will always have to annoy you 😄

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 30, 2020 •
edited
Loading

@Croydon I'm close to provide a PR with last abseil release, where cmake config file is (more or less naively) parsed in package method to produce a json file installed somewhere in lib folder, and read in package_info.

@mathbunnyru
Copy link
Contributor

Thanks @SpaceIm, it sounds awesome.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 30, 2020

#3050

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prince-chrismc prince-chrismc prince-chrismc left review comments

@Croydon Croydon Croydon requested changes

@danimtb danimtb danimtb approved these changes

@uilianries uilianries uilianries approved these changes

@SSE4 SSE4 SSE4 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@SpaceIm @conan-center-bot @Croydon @prince-chrismc @danimtb @mathbunnyru @uilianries @SSE4