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

Issue #910, #48, #102 - Require Python 3.9 in RPM package #398

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jiridanek
Copy link
Contributor

@jiridanek jiridanek commented Apr 29, 2022

As the linked issue explains, Python 3.6 is out-of-upstream-support. Is it possible to use a newer Python on ubi8? Yes! Is it advisable? Not sure. Can it be done for RPMs too? Yes! It would be easy if there was python-proton packaged for python39. But there is not one.

The solution needed for the RPM is a real mess (and it is only a good fortune that it plugs into an already existing mess with skupper-router-internal and sys.path modifications that the router already implements).

I think that if Python >3.6 was deemed worth the effort, it would be necessary to either have a python39-qpid-proton RPM package (which does not have file conflicts with the python3-qpid-proton package) or decide only package the skrouterd in a RPM and distribute the sktools using pip, or something along these lines.

I am not myself convinced this Python 3.9 is a good idea, overall. It is interesting, though, that CentOS/ubi has support for using non-system version of Python for RPMs. On CentOS, there is even automatic rewrite of the #! in scripts to use Python 3.9 (or whichever version is chosen).

@jiridanek jiridanek linked an issue Apr 29, 2022 that may be closed by this pull request
@jiridanek jiridanek force-pushed the jd_2022_03_15_rpm_python branch 2 times, most recently from 4d787af to 2950de8 Compare May 2, 2022 06:55
@jiridanek jiridanek changed the title Issue #48, #102 - Require Python 3.9 (and see how it goes) [experiment] Issue #48, #102 - Require Python 3.9 (and see how it goes) May 18, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2022

Codecov Report

Merging #398 (1a7947a) into main (b9ffb33) will decrease coverage by 0.08%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
- Coverage   78.00%   77.93%   -0.08%     
==========================================
  Files         239      239              
  Lines       60753    60754       +1     
  Branches     5585     5585              
==========================================
- Hits        47392    47348      -44     
- Misses      10748    10781      +33     
- Partials     2613     2625      +12     
Flag Coverage Δ
pysystemtests 87.42% <ø> (-0.09%) ⬇️
pyunittests 54.44% <ø> (ø)
systemtests 71.61% <ø> (-0.07%) ⬇️
unittests 27.19% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
calculator 30.78% <ø> (+<0.01%) ⬆️
systemtests 78.51% <ø> (-0.08%) ⬇️

@jiridanek jiridanek force-pushed the jd_2022_03_15_rpm_python branch 2 times, most recently from 7c7e13e to 7c8ec14 Compare October 27, 2022 19:50
@jiridanek jiridanek force-pushed the jd_2022_03_15_rpm_python branch 5 times, most recently from aafc473 to 6945406 Compare January 21, 2023 11:03
@jiridanek jiridanek changed the title [experiment] Issue #48, #102 - Require Python 3.9 (and see how it goes) Issue #910, #48, #102 - Require Python 3.9 in RPM package Jan 21, 2023
@jiridanek
Copy link
Contributor Author

The idea is that as we need to build skupper RPMs, we cannot depend on python39-qpid-proton (because, I am assuming, it is not yet available). Therefore, it is necessary to build the python binding for proton here, and ship it (in rpm skupper-router-common). That is the cause for most of the changes in the PR diff

Next, replacing python3 with python39 constitutes the rest of the diff.

@jiridanek
Copy link
Contributor Author

I tried building this on COPR, which showed the following repeated test fail

======================================================================
FAIL: test_80_stats (system_tests_tcp_adaptor.TcpAdaptor)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/skupper-router/tests/system_tests_tcp_adaptor.py", line 1337, in test_80_stats
    self.assertEqual(es_inta_connections_opened, 7)
AssertionError: 6 != 7

e.g. https://download.copr.fedorainfracloud.org/results/jdanek/skupper-router/epel-8-aarch64/05281012-skupper-router/builder-live.log.gz

I am wanting to dismiss this as just some timing issue on lowpowered build machine, but it happens everywhere!

The build results are at https://copr.fedorainfracloud.org/coprs/jdanek/skupper-router/build/5281012/, the failed states mean failed tests, not failed compilation

# check ctest
BuildRequires: cyrus-sasl-plain

# proton-c requirements
BuildRequires: openssl-devel
BuildRequires: cyrus-sasl-devel
# python-qpid-proton requirements
BuildRequires: swig
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this is now out of date, proton python is using cffi; but early this week it still worked with main

@jiridanek
Copy link
Contributor Author

@ganeshmurthy
Copy link
Contributor

Yes please, thanks.

@ganeshmurthy
Copy link
Contributor

@jiridanek Can this PR can be closed in favor of #1120 ?

@jiridanek
Copy link
Contributor Author

@jiridanek Can this PR can be closed in favor of #1120 ?

I like the idea of building python-qpid-proton as part of this RPM build, so that we would no longer mix embedded proton with nonembedded proton. That's something I think can still be used.

What was the long-term plan about embedding proton, anyways? I think that it will be done for the foreseeable future, yes? It is not something we want to abandon. In that case, I'd completely remove dependency on python3-qpid-proton and build the proton python here, and embed it for private use by skstat and skmanage.

Wdyt?, thanks

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.

Remove downgrade of the python version in GHA configuration
3 participants