posix_ipc icon indicating copy to clipboard operation
posix_ipc copied to clipboard

feature: enable overriding system probe

Open ramonnr opened this issue 1 year ago • 5 comments

I'm involved in a project that is looking at using posix-ipc in a cross build environment. In the buildsystem we're using (yocto) the probe.py causes some problems, where the host (x86) does not have the same capabilities as the target (arm64). It feels a bit hacky relying on env vars, but I couldn't really see any less intrusive way of making the probe conditional. I targeted master as it's a few commits ahead of develop, not sure if you want PR's submitted to develop anyway.

Let me know if there are any cleanups or if there is another prefered way of achieving this you'd rather see.

ramonnr avatar Apr 17 '23 15:04 ramonnr

Hi @ramonnr, thanks for the PR. Are you aware that the prober won't overwrite probe_results.h if it already exists? You can edit probe_results.h to contain whatever settings you like and then re-run setup.py.

osvenskan avatar Apr 18 '23 18:04 osvenskan

I was not aware! I could add a line in the readme about that instead then. The linker flag would still need to be injected somehow. Thinking about it I only see two options for the -lrt flag, either pass a env var or set a magic comment in the probe_results.h file which prober.py could look for. Any of these seem reasonable enough and/or preferable to you?

ramonnr avatar Apr 19 '23 06:04 ramonnr

I was not aware! I could add a line in the readme about that instead then.

That's a good idea. This message gets written to the top of every probe_results.h, so whatever goes into the readme should use similar language --

This header file was generated when you ran setup. Once created, the setup
process won't overwrite it, so you can adjust the values by hand and
recompile if you need to.

The linker flag would still need to be injected somehow. Thinking about it I only see two options for the -lrt flag, either pass a env var or set a magic comment in the probe_results.h file which prober.py could look for. Any of these seem reasonable enough and/or preferable to you?

The approach I would take is to add the ability to pass an option to setup.py. --use-realtime-lib immediately comes to mind, but then there will probably be someone who wants to do the opposite of what you're doing and that will require adding --no-realtime-lib.

What do you think about a --libraries flag that takes a list of comma separated values? If specified, setup.py uses that list, otherwise it uses the existing logic here: https://github.com/osvenskan/posix_ipc/blob/develop/setup.py#L47

FYI, PRs should be against develop, not master. You're right that master is ahead of develop and I need to figure out how to correct that, but the commits are just me merging develop into master. Right now there's zero code difference between the two branches.

osvenskan avatar Apr 19 '23 15:04 osvenskan

That's a good idea. This message gets written to the top of every probe_results.h, so whatever goes into the readme should use similar language --

This header file was generated when you ran setup. Once created, the setup
process won't overwrite it, so you can adjust the values by hand and
recompile if you need to.

Sounds good to me.

The approach I would take is to add the ability to pass an option to setup.py. --use-realtime-lib immediately comes to mind, but then there will probably be someone who wants to do the opposite of what you're doing and that will require adding --no-realtime-lib.

What do you think about a --libraries flag that takes a list of comma separated values? If specified, setup.py uses that list, otherwise it uses the existing logic here: https://github.com/osvenskan/posix_ipc/blob/develop/setup.py#L47

I've done some testing with adding additional arguments to setup.py with argparse, this seems to break the invocation of setuptools though. One option here is enabling a user to supply a setup.cfg file which can be parsed with setuptools.config.read_configuration, but this adds some complexity since distutils are still conditionally imported. So I'm not super happy with that approach either. Additionally it's marked as deprecated.

I'm thinking the best path forward might be to migrate to using a pyproject.toml. Not sure here, I'll do some thinking over the weekend and will get back to you with a suggestion.

FYI, PRs should be against develop, not master. You're right that master is ahead of develop and I need to figure out how to correct that, but the commits are just me merging develop into master. Right now there's zero code difference between the two branches.

I'll retarget my PR when I have a new commit I believe in :)

You can probably rebase develop on master to get the merge commits into develop

ramonnr avatar Apr 21 '23 09:04 ramonnr

I would be open to converting a pyproject.toml. This project is 15 years old and some of it hasn't changed since its inception. I mostly take the approach of "if it ain't broke don't fix it" so useful infrastructure enhancements often wait a long time to happen.

osvenskan avatar Apr 21 '23 12:04 osvenskan