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

Feature/nsync #6085

Merged
merged 32 commits into from
Sep 25, 2021
Merged

Feature/nsync #6085

merged 32 commits into from
Sep 25, 2021

Conversation

CAMOBAP
Copy link
Contributor

@CAMOBAP CAMOBAP commented Jun 29, 2021

Specify library name and version: nsync/1.23.0

nsync is a dependency for onnxruntime receipt, which is in progress


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

@CAMOBAP CAMOBAP mentioned this pull request Jun 29, 2021
4 tasks
@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.

@stale
Copy link

stale bot commented Jul 30, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 30, 2021
@stale stale bot removed the stale label Aug 19, 2021
@conan-center-bot

This comment has been minimized.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Aug 19, 2021

More information from gdb

Starting program: /test/all/test_package/build/04a89eb3fd1ca69c0373770cfc8222ea2544fde6/bin/example_c
warning: Error disabling address space randomization: Operation not permitted
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x0000000000402c17 in nsync_dll_splice_after_ ()

https://github.com/google/nsync/blob/master/internal/dll.c#L61 - do be investigated ...

@uilianries
Copy link
Member

@CAMOBAP All DLL files should be copied to bin folder, not lib.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Sep 16, 2021

@uilianries yep just fixed this. But how those DLL will be found in runtime? Should we add bin to libdirs?

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Sep 16, 2021

@uilianries thanks a lot for your help! looks like all done now

@CAMOBAP CAMOBAP requested a review from uilianries September 16, 2021 18:41
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Almost good to go. only small suggestions

@conan-center-bot

This comment has been minimized.

@CAMOBAP
Copy link
Contributor Author

CAMOBAP commented Sep 17, 2021

Done

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.

One more 🙃

Move base_path to conandata.yaml

Co-authored-by: Chris Mc <[email protected]>
@conan-center-bot

This comment has been minimized.

uilianries
uilianries previously approved these changes Sep 20, 2021
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM thank you!!

ar_dest = \
"ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} " \
"COMPONENT Development"
rt_dest = 'RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd apply this change regardless of the OS and build type, even if only makes sense there.
Also keep in mind you're removing the COMPONENT, I don't know the implications of this in this particular case.

Copy link
Contributor Author

@CAMOBAP CAMOBAP Sep 21, 2021

Choose a reason for hiding this comment

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

Per documentation in https://cmake.org/cmake/help/latest/command/install.html:

COMPONENT
Specify an installation component name with which the install rule is associated, such as "runtime" or "development". During component-specific installation only install rules associated with the given component name will be executed. During a full installation all components are installed unless marked with EXCLUDE_FROM_ALL. If COMPONENT is not provided a default component "Unspecified" is created. The default component name may be controlled with the CMAKE_INSTALL_DEFAULT_COMPONENT_NAME variable.

Because we don't perform component-specific installation probably this is not the case for us.

As another solution, I think we can safely add COMPONENT RuntimeLibraries

prince-chrismc
prince-chrismc previously approved these changes Sep 21, 2021
Fix typo in receipt description

Co-authored-by: Eric Riff <[email protected]>
@CAMOBAP CAMOBAP dismissed stale reviews from prince-chrismc and uilianries via b48c151 September 21, 2021 06:27
@conan-center-bot
Copy link
Collaborator

All green in build 38 (b48c151326ed97e026824eda09d63d1639609d0b):

  • nsync/1.23.0@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 4d50820 into conan-io:master Sep 25, 2021
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.

8 participants