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

Disable building of shared library needed by collect_stats.py by default #11

Merged
merged 3 commits into from Jan 12, 2021

Conversation

shunghsiyu
Copy link
Member

The _phoebe.abi3.so shared library is used by collect_stats.py to retrieve various system metric; and the shared library itself requires python3-cffi to generate the binding (_phoebe.c).

On Linux distribution that doesn't come with a python3-cffi package is becomes a problem, while there are other ways to install python3-cffi (e.g. pip3 install cffi), doing so it not ideal when building the phoebe package itself.

Thus the easiest way to deal with the problem is disable building of _phoebe.abi3.so by default so packaging doesn't need python3-cffi, and explicitly enable it in our CI.

@shunghsiyu
Copy link
Member Author

shunghsiyu commented Jan 8, 2021

@dcermak since you know meson and GitHub action much better, can you take a quick glance?

Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

This should be done via a meson feature, or even better, the build of the shared library should depend on the existence of the ffi module

@shunghsiyu
Copy link
Member Author

build of the shared library should depend on the existence of the ffi module

Interesting, haven't thought of that, let me try to do so instead.

@shunghsiyu shunghsiyu marked this pull request as draft January 8, 2021 10:39
@dcermak
Copy link
Collaborator

dcermak commented Jan 8, 2021

I can send a PR instead, if you don't like to mess with this.

@shunghsiyu
Copy link
Member Author

I'm actually fine with this; finally got a chance to learn meson with someone who knows it ;)

@dcermak
Copy link
Collaborator

dcermak commented Jan 8, 2021

I'm actually fine with this; finally got a chance to learn meson with someone who knows it ;)

you'll need this: https://mesonbuild.com/Python-module.html#find_installation and https://mesonbuild.com/Build-options.html#features

Use the 'module' argument of find_installation() to check whether Python's CFFI
module is installed. If it is then build the shared library that is needed by
collect_stats.py, which is a script used to collect system metrics for AI model
training; if a Python installation with CFFI module is not found, then don't
bother building the shared library. (Note: collect_stats.py is generally not
needed on production/deployment)

A major behind this is that there are SLES distributions that does not provide
the python3-cffi package, which makes RPM packaging difficult. With building of
the shared library (requires python3-cffi) determined dynmically, we can get
away with the missing python3-cffi package.
@shunghsiyu
Copy link
Member Author

Now use find_installation('python3', required : false, modules : ['cffi']) instead to find a Python installation with CFFI module available.

I was hoping to get rid of the if conditional and replace it with required inside custom_target() and shared_library(). Sadly custom_target() doesn't seem to support the required argument.

@@ -1,14 +1,16 @@
prog_python = import('python').find_installation('python3')
prog_python = import('python').find_installation('python3', required : false, modules : ['cffi'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could try to use the required value from a feature, so that users can enforce this module being present (and fail the configure step).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks for the tips!

So that users can enforce the module needed by collect_stats.py being present,
and fail the configure step if otherwise.
@shunghsiyu shunghsiyu marked this pull request as ready for review January 12, 2021 02:41
@mvarlese
Copy link
Collaborator

Thanks!

@mvarlese mvarlese merged commit 04ee99c into SUSE:main Jan 12, 2021
shunghsiyu added a commit to shunghsiyu/phoebe that referenced this pull request Jan 12, 2021
This is a continuation of work done in SUSE#11. While building of _phoebe.abi3.so
can be toggled and dynamically determined, the integration test is not aware of
that; this works right now because testing of collect_stat.py is disabled right
now due to SUSE#2.

Break up what was run_tests.sh into setup.sh (majority of run_tests.sh goes
there) and two test() defined in /tests/meson.build, one for phoebe and another
for collect_static.py. Since meson runs the tests in parallel by default,
setup.sh is given an arbitrary high value with is_parallel == false to ensure it
is ran before anything else.
shunghsiyu added a commit to shunghsiyu/phoebe that referenced this pull request Jan 13, 2021
This is a continuation of work done in SUSE#11. While building of _phoebe.abi3.so
can be toggled and dynamically determined, the integration test is not aware of
that; this works right now because testing of collect_stat.py is disabled right
now due to SUSE#2.

Break up what was run_tests.sh into setup.sh (majority of run_tests.sh goes
there) and two test() defined in /tests/meson.build, one for phoebe and another
for collect_static.py. Since meson runs the tests in parallel by default,
setup.sh is given an arbitrary high value with is_parallel == false to ensure it
is ran before anything else.
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.

None yet

3 participants