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 recipe for CERN ROOT data analysis framework. #3732

Merged
merged 38 commits into from
Feb 8, 2021
Merged

Add recipe for CERN ROOT data analysis framework. #3732

merged 38 commits into from
Feb 8, 2021

Conversation

davehadley
Copy link
Contributor

Specify library name and version: root/v6-22-02

  • 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 pull request adds a recipe for the ROOT data analysis framework (https://root.cern/).

ROOT can only be built in shared mode at present. This violates #KB-H050: "DEFAULT SHARED OPTION VALUE" and is being discussed in conan-io/hooks#252. For now I have removed the shared default to allow this hook check to pass. However, depending on the outcome of the referenced issue, this option may need to be added back in.

@CLAassistant
Copy link

CLAassistant commented Nov 29, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@davehadley davehadley mentioned this pull request Nov 29, 2020
Open
@conan-center-bot

This comment has been minimized.

Sign in to view
@davehadley
Copy link
Contributor Author

davehadley commented Nov 30, 2020 •
edited
Loading

@prince-chrismc thank you for your helpful comments. I have responded in line above and will work on addressing them.

@davehadley
Minimal changes to fix CI failure.
34e5b72 
Fixes two classes of CI failure:

(1) ROOT6 requires C++ standard >= 11.
This is now checked with tools.check_min_cppstd.

(2) Seg fault when building with clang dues LLVM library loading issues.
See: https://root-forum.cern.ch/t/root-with-geant4-llvm-problems/21960
@conan-center-bot

This comment has been minimized.

Sign in to view
@SSE4 SSE4 requested a review from uilianries December 3, 2020 06:37
@davehadley
Implement improvements for PR #3732
7f503ea 
Implement improvements suggested by @prince-chrismc including:

(1) Remove default options for dependent packages.

(2) Remove all f-strings for python 2 compatibility.

(3) Cache CMake object.

(4) Use dependencies from CCI where available.

(5) Reduce size of test_package.

(6) Apply version check boiler plate from conan-io/conan#8002.
@conan-center-bot

This comment has been minimized.

Sign in to view
prince-chrismc
prince-chrismc reviewed Dec 4, 2020
View reviewed changes
recipes/root/all/conanfile.py Outdated Show resolved Hide resolved
recipes/root/all/conanfile.py Outdated Show resolved Hide resolved
recipes/root/all/test_package/CMakeLists.txt Outdated Show resolved Hide resolved
recipes/root/all/conanfile.py Outdated Show resolved Hide resolved
recipes/root/all/conanfile.py Outdated Show resolved Hide resolved
@davehadley
Implement PR #3732 review improvements
c4a2736 
(1) Remove unused variable.

(2) Prefer class variables over instance variables.

(3) Move source patching from source method to build method.

(4) String formatting style changes.

(5) Use tools to remove files over in-recipe implementation.
@conan-center-bot

This comment has been minimized.

Sign in to view
@davehadley
Fix typo in 66718b6.
df3021f 
This should fix the failing CI clang-5 build.
@conan-center-bot

This comment has been minimized.

Sign in to view
@davehadley davehadley marked this pull request as ready for review December 6, 2020 14:06
prince-chrismc
prince-chrismc previously approved these changes Dec 6, 2020
View reviewed changes
recipes/root/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

Sign in to view
prince-chrismc
prince-chrismc suggested changes Jan 31, 2021
View reviewed changes
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

I think this is enough for a first pass.

Recipes do not need to be perfect and we can comeback to fix/add things as needed

One small editorial

recipes/cern-root/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

Sign in to view
prince-chrismc
prince-chrismc previously approved these changes Feb 1, 2021
View reviewed changes
@davehadley
Adding code review suggestions by @madebr
d0a1795 
This should solve build issues on gcc and clang when libcxx != libstdc++11.

See:
<#3732 (comment)>
@conan-center-bot

This comment has been minimized.

Sign in to view
@madebr
Copy link
Contributor

madebr commented Feb 2, 2021

I think the latest error is due to a missing include of cstdlib.
https:/root-project/root/blob/master/core/foundation/src/FoundationUtils.cxx

@davehadley
Copy link
Contributor Author

I think the latest error is due to a missing include of cstdlib.
https:/root-project/root/blob/master/core/foundation/src/FoundationUtils.cxx

Yes. This seems to be a portability problem in ROOT.

For the record the compiler error is error: no member named 'getenv' in the global namespace with libcxx==libc++ on clang. For example: https://c3i.jfrog.io/c3i/misc/logs/pr/3732/23/cern-root/v6-22-06/dd17abfadd29ee644d16d6952844a77cde4e6cf4/create_stderr.txt

There is a stack overflow post that gives a good explanation of this: https://stackoverflow.com/questions/29551755/why-getenv-can-get-name-resolved-without-a-std.

@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot
Copy link
Collaborator

All green in build 26 (fd02ab0aea1b998db17a646e93c454a78651e4d7)! 😊

prince-chrismc
prince-chrismc approved these changes Feb 3, 2021
View reviewed changes
Copy link
Contributor

@prince-chrismc prince-chrismc 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 ff86c41 into conan-io:master Feb 8, 2021
@davehadley davehadley deleted the root/v6-22-02 branch February 8, 2021 18:12
@SpaceIm SpaceIm mentioned this pull request Feb 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Croydon Croydon Croydon left review comments

@madebr madebr madebr left review comments

@SpaceIm SpaceIm SpaceIm left review comments

@SSE4 SSE4 SSE4 approved these changes

@uilianries uilianries uilianries approved these changes

@prince-chrismc prince-chrismc prince-chrismc approved these changes

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

Successfully merging this pull request may close these issues.
9 participants
@davehadley @CLAassistant @conan-center-bot @madebr @prince-chrismc @SSE4 @Croydon @uilianries @SpaceIm