-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fixed #186: Add rpm .spec to skupper-router project #189
Conversation
@mcressman @ajssmith Do you think such basic rpm .spec is sufficient for upstream uses, or is there something more that has to be done? Do we want to also build qpid-proton static libraries as part of this spec file? |
skupper-router.spec
Outdated
%check | ||
%ctest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may not be a good idea, I heard multiple people talking against this. Easy to skip with --nocheck
, though.
c53cddc
to
6430ac7
Compare
- name: Check that skrouterd works | ||
run: | | ||
skrouterd -c /dev/empty |& grep "Configuration file could not be opened" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, this should be testing each rpm package independently ("Does -server
package work without -tools
being installed?") and more in depth. Sadly, the -tests
package is actually broken and the system tests don't run against installed skrouterd (this is known ENTMQIC issue). For now, what I have is good enough, I think. It checks that Python internal module can be loaded, because that is happening before router accesses config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you open a corresponding GH skupper-router issue for this -tests package breakage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skupper-router.spec.rpkg
Outdated
# TODO: name? | ||
%package server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, the packages were called qpid-dispatch-router
, qpid-dispatch-tools
, ... If the base name is to be skupper-router
, we can't have skupper-router-router
. I resolved this by calling it -server
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcressman @fgiorgetti thoughts?
.github/workflows/build.yaml
Outdated
man skrouterd | ||
man skstat | ||
man skmanage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The man pages are somewhat outdated regarding skupper-router changes. The synopsis talk about skrouter being good for AMQP, disregarding the differences from old dispatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skupper-router.spec.rpkg
Outdated
Requires: python3 | ||
Requires: qpid-proton-c >= %{proton_minimum_version} | ||
Requires: libwebsockets >= %{libwebsockets_minimum_version} | ||
Requires: libnghttp2 >= %{libnghttp2_minimum_version} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here (and for -tools) there probably should be a Requires: for cyrus-sasl-plain
. That is needed to pass %check
(due to system_tests_sasl_plain
) and actually I think users should have this package ready since they are very likely to need it.
If you disagree this should be runtime dep, it can be made a build dep, maybe condition it on whether %check is required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put the dep on -tests package! That should be the smallest possible change. Still, I think that users should not be allowed to install the router without plain sasl mechanism. It is usually needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, putting it on -tests package. Hopefully this will work the way I expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not work as expected, it has to be a BuildRequires
. There is an enhancement proposal for rpmbuild which got stuck https://bugzilla.redhat.com/show_bug.cgi?id=1134397.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed now.
c8c5658
to
f66f45b
Compare
skupper-router.spec.rpkg
Outdated
# ctest | ||
BuildRequires: cyrus-sasl-plain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to add this, so I can run (some) tests in Fedora COPR. The results look ok, with some flakes here and there
https://copr.fedorainfracloud.org/coprs/jdanek/skupper-router/build/3821395/
I reported missing Qpid Proton RPM on EPEL 9 at https://issues.apache.org/jira/browse/PROTON-2523
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea but to be honest all this RPM-fu is way beyond my expertise. We should get @fgiorgetti @mcressman involved for their approval.
f3305bb
to
60de4b8
Compare
skupper-router.spec.rpkg
Outdated
%{?fedora:BuildRequires: python3.9} | ||
%{?rhel:BuildRequires: python39-devel} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is attempt to use Python 3.9 everywhere, as per #48.
ModuleNotFoundError: No module named 'proton'
That happens because python3-qpid-proton
RPM installs Proton only for the system-default Python. I should've thought of that! Using static embedded build of Proton will not completely help, because the spec that @mcressman has now still needs the RPM package to do, ..., things.
Furthermore, I am not completely positive on the exact procedure to use the 3.9 Python in Fedora and CentOS. I asked on StackOverflow ;P https://stackoverflow.com/questions/71782123/how-do-i-install-python39-rpm-macros-for-fedora-35
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be resolved. We will build Qpid Proton statically as part of this RPM build anyways, which means that the python proton part can be built as well. I am not sure where to put it, though, so that it is out of the way of the user of the system, but still findable for the router. Seems to me that nonstandard location then requires some modifications in the router so that it will look there. (It can be small changes to PYTHONPATH setting, or note the -I
switch for skrouterd... feels messy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempt to use Python 3.9 everywhere is considered abandoned, because it was decided so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'll bring it up again when I'll know how to implement it cleanly; It should become easier when Proton build is embedded in building this RPM)
…os8 avoids cmake warning
60de4b8
to
52e3561
Compare
This is a very basic rpm .spec file, which nonetheless can be compiled and installed.
Tested with
rpkg local --nocheck sudo dnf install /tmp/rpkg/skupper-router-31-8gtb5wwt/x86_64/skupper-router-0.0.git.3483.7cc6a1d8.dirty.1i20wb-2.fc35.x86_64.rpm skrouterd