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

Fix Python package/module name collision #2438

Merged
merged 5 commits into from Jun 18, 2019

Conversation

@MrAnno
Copy link
Member

commented Dec 1, 2018

Previously, syslogng was a package and a module name at the same time, breaking the interactive debugger.

C-side Python objects have been moved into the _syslogng module, and these objects have been published in the syslogng package.

From now on, the main module runs in the _syslogng_main module instead of _syslogng.

The installpath of pylib has been changed to lib/syslog-ng/python. This path is automatically appended to PYTHONPATH. Relocation is also supported using the SYSLOGNG_PREFIX env variable.

Fixes #2364

@lbudai lbudai added the in progress label Dec 1, 2018
@furiel furiel self-requested a review Dec 1, 2018
@MrAnno MrAnno force-pushed the MrAnno:python-syslogngimport branch from 6f57255 to 37477f4 Dec 1, 2018
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2018

Build FAILURE

@MrAnno MrAnno changed the title python: fix package/module name collision [WIP] python: fix package/module name collision Dec 1, 2018
@MrAnno MrAnno force-pushed the MrAnno:python-syslogngimport branch 2 times, most recently from d35d082 to 5d33fc8 Dec 1, 2018
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2018

Build FAILURE

@MrAnno

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

Automatically appending PYTHONPATH with the corresponding path is not as clean as I thought.

The path is "generated" by setup.py during installation, however, we have the path of every installed file in builddir/modules/python/pylib/install-manifest.txt.

The Java binding's module-dir was a bit easier, because it is a hard-coded path, it does not change based on binding versions (for example, syslog-ng/ose/install/lib/python3.7/site-packages/).

@furiel

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

I feel this PR is a little bit stuck. Just an idea: we could move the debugger to _syslogng package instead, to avoid collision. It is far from optimal solution, as there are other benefits if we could write code in python for the c binding. But as quick workaround to get the debugger running, it might be enough.

@furiel

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

setup.py has a --install-lib option that can override the installation directory entirely. That means the path would not depend on python version. With that we could force a known installation path, so we can update PYTHONPATH correctly.

For example after adding --install-lib=/tmp/test to SetupPyLib target, after the installation I got

/tmp/test/
├── syslogng
│   ├── debuggercli
│   │   ├── choicecompleter.py
│   │   ├── commandlinelexer.py
│   │   ├── completerlang.py
│   │   ├── completer.py
│   │   ├── debuggercli.py
│   │   ├── debuglang.py
│   │   ├── getoptlexer.py
│   │   ├── __init__.py
│   │   ├── langcompleter.py
│   │   ├── lexer.py
│   │   ├── lexertoken.py
│   │   ├── macrocompleter.py
│   │   ├── __pycache__
│   │   │   ├── choicecompleter.cpython-35.pyc
│   │   │   ├── commandlinelexer.cpython-35.pyc
│   │   │   ├── completer.cpython-35.pyc
│   │   │   ├── completerlang.cpython-35.pyc
│   │   │   ├── debuggercli.cpython-35.pyc
│   │   │   ├── debuglang.cpython-35.pyc
│   │   │   ├── getoptlexer.cpython-35.pyc
│   │   │   ├── __init__.cpython-35.pyc
│   │   │   ├── langcompleter.cpython-35.pyc
│   │   │   ├── lexer.cpython-35.pyc
│   │   │   ├── lexertoken.cpython-35.pyc
│   │   │   ├── macrocompleter.cpython-35.pyc
│   │   │   ├── readline.cpython-35.pyc
│   │   │   ├── syslognginternals.cpython-35.pyc
│   │   │   ├── tablexer.cpython-35.pyc
│   │   │   ├── templatelang.cpython-35.pyc
│   │   │   ├── templatelexer.cpython-35.pyc
│   │   │   └── tflang.cpython-35.pyc
│   │   ├── readline.py
│   │   ├── syslognginternals.py
│   │   ├── tablexer.py
│   │   ├── templatelang.py
│   │   ├── templatelexer.py
│   │   └── tflang.py
│   ├── __init__.py
│   └── __pycache__
│       └── __init__.cpython-35.pyc
└── syslogng-1.0-py3.5.egg-info
MrAnno added 2 commits Jun 7, 2019
Signed-off-by: László Várady <laszlo.varady@balabit.com>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
@MrAnno MrAnno force-pushed the MrAnno:python-syslogngimport branch from 5d33fc8 to e0040ed Jun 7, 2019
@MrAnno

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

@furiel Thanks.

I've used the relocation API, because SYSLOGNG_PREFIX is actively used in syslog-ng PE.

There is something wrong with configs containing inline Python code (python_evaluate_global_code), I'll fix that before removing the WIP flag.

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Build FAILURE

@MrAnno MrAnno force-pushed the MrAnno:python-syslogngimport branch 2 times, most recently from 9cbbee6 to b8ba54d Jun 7, 2019
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Build FAILURE

@MrAnno MrAnno force-pushed the MrAnno:python-syslogngimport branch from b8ba54d to 900d67e Jun 7, 2019
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Build FAILURE

MrAnno added 2 commits Jun 7, 2019
By default, the path is lib/syslog-ng/python.

Relocation is supported using the SYSLOGNG_PREFIX env variable.

Signed-off-by: László Várady <laszlo.varady@balabit.com>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
@MrAnno MrAnno force-pushed the MrAnno:python-syslogngimport branch from 900d67e to 83ec438 Jun 7, 2019
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Build FAILURE

@MrAnno

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

It seems the _syslogng module is also overloaded.
It is used to store C-side Python objects and to load and run inline Python code too.

When executing inline Python code, _syslogng is removed from the module registry and it is recreated after every reload (_py_construct_main_module). This removes all C-side objects that are registered in _py_init_interpreter.

I'll just rename the main module to _syslogng_main to resolve this issue.

@MrAnno MrAnno changed the title [WIP] python: fix package/module name collision Fix Python package/module name collision Jun 7, 2019
@MrAnno MrAnno removed the in progress label Jun 7, 2019
@MrAnno MrAnno added this to the syslog-ng-3.22 milestone Jun 7, 2019
The main module (used for inline Python code) is recreated between reloads,
removing all internal Python objects that were registered in
_py_init_interpreter().

To avoid this issue, the main module has been renamed from _syslogng to
_syslogng_main.

Signed-off-by: László Várady <laszlo.varady@balabit.com>
@MrAnno MrAnno force-pushed the MrAnno:python-syslogngimport branch from 6854c47 to 182e0b0 Jun 7, 2019
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Build SUCCESS

@Kokan
Kokan approved these changes Jun 12, 2019
@bazsi
bazsi approved these changes Jun 16, 2019
Copy link
Collaborator

left a comment

Thanks for solving this issue. Now there's a point in improving the debugger :)

@furiel furiel removed their request for review Jun 18, 2019
@Kokan Kokan merged commit b362324 into syslog-ng:master Jun 18, 2019
4 checks passed
4 checks passed
Kira-starter Build SUCCESS
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: Python No new or fixed alerts
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 21, 2019
Follow-up for syslog-ng#2438

Signed-off-by: László Várady <laszlo.varady@balabit.com>
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 21, 2019
Follow-up for syslog-ng#2438

Signed-off-by: Peter Czanik <peter@czanik.hu>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 21, 2019
Follow-up for syslog-ng#2438

Signed-off-by: Peter Czanik <peter@czanik.hu>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 21, 2019
Follow-up for syslog-ng#2438

Signed-off-by: László Várady <laszlo.varady@balabit.com>
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 21, 2019
Follow-up for syslog-ng#2438

Signed-off-by: Peter Czanik <peter@czanik.hu>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 24, 2019
Follow-up for syslog-ng#2438

Signed-off-by: László Várady <laszlo.varady@balabit.com>
MrAnno pushed a commit to MrAnno/syslog-ng that referenced this pull request Jun 24, 2019
Follow-up for syslog-ng#2438

Signed-off-by: Peter Czanik <peter@czanik.hu>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 24, 2019
Follow-up for syslog-ng#2438

Signed-off-by: László Várady <laszlo.varady@balabit.com>
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 24, 2019
Follow-up for syslog-ng#2438

Signed-off-by: Peter Czanik <peter@czanik.hu>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
MrAnno added a commit to MrAnno/syslog-ng that referenced this pull request Jun 24, 2019
Follow-up for syslog-ng#2438

Signed-off-by: Peter Czanik <peter@czanik.hu>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.