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

Add/ignition math #4045

Merged
merged 27 commits into from
Jan 13, 2021
Merged

Add/ignition math #4045

merged 27 commits into from
Jan 13, 2021

Conversation

prince-chrismc
Copy link
Contributor

Specify library name and version: ignition-math/6.7.0

  • 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.

This is to save all the hard work we put in #3215

Juan Oxoby and others added 23 commits October 15, 2020 18:20
Add ignition-math
dce7cca
Formatting
906c403
Change version_mejor approach
6edd680
Address review comments
6245a32
Add import ConanInvalidConfiguration
6ee8253
del fPIC in Windows
629449e
Add shared and static support
c56041e
Add import glob
ba824b6
Unconditially remove files
a95413f
Address review comments
5525dcc
Remove fPIC option since it's always true
6f40994
Change test_package.cpp
e41fc75
Change test CMakeLists
a9974d7
Add regex patch
3b11e57
Add friends function Angle definition patch
4e68918
Revert "Add friends function Angle definition patch"
e846873 
This reverts commit 4e68918.
Explicitly link libc++
f659367
Revert "Explicitly link libc++"
9630d6c 
This reverts commit f659367.
@madebr
Don't run git clone + support fPIC + create components (#1)
8607d3e
Add cmake_wrapper
2f6cca7
@joxoby @madebr
Update recipes/ignition-math/all/CMakeLists.txt
cdee788 
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
@madebr
ignition-math: use ignition-cmake (#2)
8dadd87
@prince-chrismc
bump to 6.7.0
eb895c0
@conan-center-bot
Copy link
Collaborator

Failure in build 1 (eb895c060d2bf6c171fc5fd5f8bf728912858679):

  • ignition-math/6.6.0
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [CONFIG.YML HAS NEW VERSION (KB-H052)] The version "6.7.0" exists in "conandata.yml" but not in "../config.yml", so it will not be built. Please update "../config.yml" to include newly added version "6.7.0". (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H052)

@conan-center-bot
Copy link
Collaborator

An unexpected error happened and has been reported. Help is on its way! 🏇

@conan-center-bot
Copy link
Collaborator

All green in build 3 (88eaf242b9f535c4718b764c034bfa33dd420327)! 😊

@github-actions github-actions bot mentioned this pull request Dec 31, 2020
Open
madebr
madebr previously approved these changes Jan 2, 2021
View reviewed changes
@SSE4 SSE4 requested a review from uilianries January 2, 2021 05:49
SSE4
SSE4 previously approved these changes Jan 2, 2021
View reviewed changes
SpaceIm
SpaceIm reviewed Jan 9, 2021
View reviewed changes
recipes/ignition-math/all/conanfile.py Outdated
self.requires("eigen/3.3.7")

def build_requirements(self):
self.build_requires("pkgconf/1.7.3")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was in the original PR 🤷‍♂️ it worked so I didn't ask 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be useless. eigen is the only dependency, and I would be surprised that underlying build files of ignition-math use pkg-config to consume eigen in a CMake based project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.. I'll test it locally since I have a few PR to follow up =)

@prince-chrismc @SpaceIm
Apply suggestions from code review
292ebe0 
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
@prince-chrismc prince-chrismc dismissed stale reviews from SSE4 and madebr via 292ebe0 January 9, 2021 23:05
@conan-center-bot
Copy link
Collaborator

All green in build 5 (a81134732dcc3b87dbeb61b973eb607e0815191a)! 😊

SpaceIm
SpaceIm reviewed Jan 9, 2021
View reviewed changes
Copy link
Contributor

@SpaceIm SpaceIm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

cosmetic suggestion: move source() before build()

recipes/ignition-math/all/conanfile.py Outdated Show resolved Hide resolved
recipes/ignition-math/all/conanfile.py Show resolved Hide resolved
@prince-chrismc @SpaceIm
Apply suggestions from code review
371aa9a 
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
@conan-center-bot
Copy link
Collaborator

All green in build 6 (371aa9a926adb076a197435268c7e9879a20e8c8)! 😊

mathbunnyru
mathbunnyru reviewed Jan 10, 2021
View reviewed changes
recipes/ignition-math/all/patches/0001-Define-time_regex-locally.patch
Comment on lines +18 to +19
- static const std::regex time_regex(
+ static const char* time_regex_str =
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with creating std::regex here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent questions....

From the original author #3215 (comment)

After a lot of pain, I was able to solve the segfault error that was happening. The problem was the static initialization of a std::regex variable: https://github.com/ignitionrobotics/ign-math/blob/51eb0640a0c7454c36fef1a3007d3db37fe9165f/include/ignition/math/Helpers.hh#L873, which I solved by creating a raw static char* and using that to initialize a local static (static inside the function) std::regex variable: https://github.com/conan-io/conan-center-index/pull/3215/files#diff-e2f831892c4567a7388d8d62242576015a59f008af8dd34c946996d15f6991e9

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see what problem it fixes and how, but good to know it fixes something :)

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 agree... I just didn't want to waste the time we put in so I wanted to keep it but I have no clue what this project is or does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, works for me 👍

@conan-center-bot conan-center-bot merged commit 6bd6f71 into conan-io:master Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathbunnyru mathbunnyru mathbunnyru left review comments

@SpaceIm SpaceIm SpaceIm approved these changes

@madebr madebr madebr approved these changes

@SSE4 SSE4 SSE4 approved these changes

@uilianries uilianries Awaiting requested review from uilianries

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@prince-chrismc @conan-center-bot @SSE4 @madebr @mathbunnyru @SpaceIm @joxoby