Skip to content
This repository was archived by the owner on Mar 24, 2026. It is now read-only.

Test for missing system_libs (KB-H043)#171

Merged
uilianries merged 17 commits into
conan-io:masterfrom
madebr:missing_systemlibs
Mar 30, 2022
Merged

Test for missing system_libs (KB-H043)#171
uilianries merged 17 commits into
conan-io:masterfrom
madebr:missing_systemlibs

Conversation

@madebr
Copy link
Copy Markdown
Contributor

@madebr madebr commented Mar 13, 2020

Test whether a recipe has all system libraries listed in cpp_info.system_libs.
This heuristic only run on Linux and Windows and can only be done for shared libraries.

WIKI:

#KB-H041: "MISSING SYSTEM LIBS"

A shared library links to some system libraries which are not set by cpp_info.system_libs in package_info.
This test can only be run on shared libraries.

@madebr
Copy link
Copy Markdown
Contributor Author

madebr commented Mar 13, 2020

@uilianries
Using this code, we can also check whether the order of the libraries in cpp_info.libs is correct.
Is that hook-worthy?

@madebr
Copy link
Copy Markdown
Contributor Author

madebr commented Mar 13, 2020

I think Windows CI fails because it needs the Visual Studio compiler and it is not set in the test section.
(test_script section in appveyor.yml)

@uilianries
Copy link
Copy Markdown
Member

@madebr

Using this code, we can also check whether the order of the libraries in cpp_info.libs is correct.

What do mean by correct? Anyway, it deserves a separated hook.

Copy link
Copy Markdown
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.

Do think about header-only libraries? I mean, in terms of test. Some of them contain system_libs.

Comment thread hooks/conan-center.py Outdated
@madebr
Copy link
Copy Markdown
Contributor Author

madebr commented Mar 13, 2020

@uilianries

What do mean by correct?

By correct, I mean correct ordering. Such that dependencies of other libraries come last in the list.

Do think about header-only libraries? I mean, in terms of test. Some of them contain system_libs.

header-only libraries require another approach.
This pr analyzes shared libraries for its dependencies, which is a fast operation.

For header-only libraries, we'ld need to grep all headers for occurrences of certain includes, which can be time and cpu intensive for larger projects.

e.g. for glibc: (dl, m, pthread and rt)

r"""#\w*include\w*([<"])((dlfcn.h)|(math.h)|(pthread.h)|(time.h))\g<1>"""

It can be done, but will be non-perfect.
Compilers might inline some function and don't require linking to an external library.

@uilianries
Copy link
Copy Markdown
Member

@uilianries

What do mean by correct?

By correct, I mean correct ordering. Such that dependencies of other libraries come last in the list.

Do think about header-only libraries? I mean, in terms of test. Some of them contain system_libs.

header-only libraries require another approach.
This pr analyzes shared libraries for its dependencies, which is a fast operation.

For header-only libraries, we'ld need to grep all headers for occurrences of certain includes, which can be time and cpu intensive for larger projects.

e.g. for glibc: (dl, m, pthread and rt)

r"""#\w*include\w*([<"])((dlfcn.h)|(math.h)|(pthread.h)|(time.h))\g<1>"""

It can be done, but will be non-perfect.
Compilers might inline some function and don't require linking to an external library.

Okay, let's update it in another occasion.

@madebr madebr force-pushed the missing_systemlibs branch from d803413 to 3a5a081 Compare April 28, 2020 15:30
@madebr
Copy link
Copy Markdown
Contributor Author

madebr commented Apr 28, 2020

@uilianries
The tests fail on Appveyor, because it cannot find the Visual Studio installation in the post_package hook.
https://ci.appveyor.com/project/ConanOrgCI/hooks/builds/32505216/job/gs8bxp78pu2y82i3#L238

E   ERROR: [HOOK - C:\projects\hooks\tests\test_hooks\conan-center\..\..\..\hooks\conan-center.py] post_package(): VS non-existing installation: Visual Studio 14

Do you have an idea what might be up?

@SpaceIm
Copy link
Copy Markdown
Contributor

SpaceIm commented Apr 29, 2020

@madebr Does this hook allow recipes to not list system libs if those system libs are not direct dependencies but instead are coming from system libs of dependencies?

@madebr
Copy link
Copy Markdown
Contributor Author

madebr commented Apr 29, 2020

@madebr Does this hook allow recipes to not list system libs if those system libs are not direct dependencies but instead are coming from system libs of dependencies?

Yes!

On Linux, it runs objdump -p $LIBRARY and parses the output.
It shows only the direct dependencies of the executables.
Whereas ldd shows the entire depenency tree.

I've got this from the Security section from the man page of ldd.

@prince-chrismc
Copy link
Copy Markdown
Contributor

I've used this tool for windows https://docs.microsoft.com/en-us/cpp/windows/understanding-the-dependencies-of-a-visual-cpp-application?view=vs-2019

There's a link to a GitHub repo with the tool

@madebr madebr force-pushed the missing_systemlibs branch from fb43ecb to 5e88ab6 Compare June 3, 2020 01:53
@SSE4
Copy link
Copy Markdown
Contributor

SSE4 commented Jul 11, 2020

@madebr I took some look, and it seems like environment is somehow corrupted, e.g. it returns None from:

program_files = get_env("ProgramFiles(x86)") or get_env("ProgramFiles")

and from:

env_var = "vs%s0comntools" % compiler_version
vs_path = os.getenv(env_var)

I am pretty sure it's corrupted somewhere inside the test, probably in _get_environ? could you double-check that?

@SSE4 SSE4 force-pushed the missing_systemlibs branch 4 times, most recently from d0f8ea0 to beb486b Compare February 17, 2022 12:13
SSE4
SSE4 previously approved these changes Feb 17, 2022
uilianries
uilianries previously approved these changes Feb 17, 2022
Copy link
Copy Markdown
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

Comment thread tox.ini
coverage: PYTESTDJANGO_COVERAGE_SRC={toxinidir}/

passenv = PYTEST_ADDOPTS USE_UNSUPPORTED_CONAN_WITH_PYTHON_2 USERNAME
passenv = PYTEST_ADDOPTS USE_UNSUPPORTED_CONAN_WITH_PYTHON_2 USERNAME ProgramFiles(x86) ProgramFiles
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice job @SSE4
How a one line change can fix (almost) everything.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it took me two years to find it: https://tox.wiki/en/latest/config.html#conf-passenv 🤦

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

well, today we learned something new at least 🤓

@SSE4 SSE4 mentioned this pull request Feb 19, 2022
Comment thread tests/requirements_test.txt Outdated
@SSE4 SSE4 dismissed stale reviews from uilianries and themself via 8559917 February 23, 2022 05:38
uilianries
uilianries previously approved these changes Mar 2, 2022
Copy link
Copy Markdown
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

@SSE4 SSE4 force-pushed the missing_systemlibs branch 2 times, most recently from 142954f to 174147b Compare March 3, 2022 16:25
@SSE4 SSE4 force-pushed the missing_systemlibs branch from 1f38e16 to 223bf4e Compare March 4, 2022 07:49
Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4 SSE4 force-pushed the missing_systemlibs branch from 223bf4e to fc5bfdd Compare March 4, 2022 07:58
Comment thread hooks/conan-center.py Outdated
SSE4 added 4 commits March 4, 2022 00:09
Signed-off-by: SSE4 <tomskside@gmail.com>
Signed-off-by: SSE4 <tomskside@gmail.com>
Signed-off-by: SSE4 <tomskside@gmail.com>
SSE4
SSE4 previously approved these changes Mar 4, 2022
@SSE4 SSE4 requested a review from uilianries March 4, 2022 09:33
@SSE4
Copy link
Copy Markdown
Contributor

SSE4 commented Mar 4, 2022

@madebr @uilianries I've added some macOS implementation, it's detecting missing frameworks.
P.S. it should be relatively safe to activate the hook - it only prints warnings. ofcourse, there could be some hard errors (exceptions) from it, but I think we can address them quickly.

Copy link
Copy Markdown
Contributor Author

@madebr madebr left a comment

Choose a reason for hiding this comment

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

I'm not 100% confident of this pr, but as @SSE4 said.
This hook only emits warnings and bugs can be fixed.

Comment thread hooks/conan-center.py Outdated
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
SSE4
SSE4 previously approved these changes Mar 4, 2022
uilianries
uilianries previously approved these changes Mar 4, 2022
@SSE4 SSE4 dismissed stale reviews from uilianries and themself via 3187417 March 30, 2022 07:17
@SSE4 SSE4 requested a review from uilianries March 30, 2022 08:45
@uilianries uilianries merged commit f20f768 into conan-io:master Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants