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

[py3] pki: add missing depedency pki-base[-python3] #336

Closed
wants to merge 2 commits into from
Closed

[py3] pki: add missing depedency pki-base[-python3] #336

wants to merge 2 commits into from

Conversation

MartinBasti
Copy link
Contributor

@MartinBasti MartinBasti commented Dec 14, 2016

FreeIPA server modules requires pki module

https://fedorahosted.org/freeipa/ticket/4985

@tiran
Copy link
Member

tiran commented Dec 15, 2016

BuildRequires has no python3 dependency and a different minimal version.

BuildRequires:  pki-base >= 10.2.1

@MartinBasti
Copy link
Contributor Author

It has Py3 Build dependency
Fixed.

@tiran
Copy link
Member

tiran commented Dec 15, 2016

What I meant with different minimal version, FreeIPA requires pki-ca and kra 10.3.5 and newer.

Requires: pki-ca >= 10.3.5-6
Requires: pki-kra >= 10.3.5-6

Is there a reason why the minimal version for the pki-base package is smaller? All Dogtag PKI packages pull in other PKI packages with exactly the same version (e.g. pki-base = %{version}-%{release}). Even more import, there is no Python 3 pki-base package for 10.2! I added the new package for 10.3. :)

Lastly pki-base just happens to provide Python 2 packages for now. In 10.4 or 10.5 we may switch over the Python 3. If you need pki Python package with guaranteed Python 2 support, then please depend on pki-base-python2 (currently provided by pki-base).

@MartinBasti
Copy link
Contributor Author

Ah my previous patch was right then. BuildRequires version is lower because build works with lower version. I can raise that version just to be sure that pylint is checking the right packages

@HonzaCholasta
Copy link
Contributor

HonzaCholasta commented Dec 16, 2016

@mbasti-rh, please don't bump BuildRequires unless it is actually necessary for the build to not fail. Raising the version does not guarantee that the package version used during build and checked by pylint will be the same as the version used during run time anyway, so the change achieves nothing.

@tiran
Copy link
Member

tiran commented Jan 18, 2017

What's the hold up here?

Martin and I discussed the necessity to raise the version requirements. Python 3 packages for PKI simply do not exist until 10.3. I don't want to depend on a non-existing package.

In case there are some issues with our CI and proper updates of build requirements, then the issue should be handled by a separate ticket.

@HonzaCholasta
Copy link
Contributor

@tiran, the dependency says >= 10.2.1, not == 10.2.1, so we are not depending on any non-existent packages.

@tiran
Copy link
Member

tiran commented Jan 18, 2017

pki-base-python3 >= 10.2.1 would mean that FreeIPA is compatible with pki-base-python2 == 10.2.1 which clearly does not exist.

@HonzaCholasta
Copy link
Contributor

That is of no concern to us. pki-base-python3 >= 10.2.1 will get us the correct package in all cases and under no circumstances will it cause an attempt to install a non-existent package. Note that pki-base-python2 >= 10.2.1 means that FreeIPA is also compatible with pki-base-python2-10.2.1.0.1.2.3, which clearly doesn't exist either, but that doesn't make the dependency wrong in any way whatsoever.

@tiran
Copy link
Member

tiran commented Jan 18, 2017

I can't see a valid argument in your response. As a co-maintainer of PKI's Python packages I'm strictly against claiming compatibility with a non-existing package version range. The PR is fine as it stands and I'm going to ACK it tomorrow. If you still like to veto against my ACK, please start a motion on the developer list and ask the rest of the team for their opinion.

You also mentioned that CI might not pick up build requirements correctly. I agree that this is a problem and must be fixed ASAP. We must be able to rely on CI tests. Please open a separate ticket.

@tkrizek
Copy link
Contributor

tkrizek commented Jan 18, 2017

I agree with @tiran here. Even though >= 10.2.1 will match the correct package, I don't think it's a good practice to use non-existent package numbers in BuildRequires.

@HonzaCholasta
Copy link
Contributor

@tiran, I'm sorry to have to point this out, but the decision whether this PR is accepted or not is not yours to make, you are not a member of the core team and this is in no way related to your integration work.

As a maintainer of IPA packages in RHEL I obviously prefer it my way. What you prefer when you co-maintain PKI Python packages is your bussiness and is not relevant here. A compromise I would be willing to accept is that the pki-base-python3 dependency will be unversioned, but pki-base-python2 must stay >= 10.2.1.

@tomaskrizek, why do you think it's a bad practice? The condition merely limits the set of package versions that satisfy the dependency, but the set is still infinite and an infinite number of non-existents packages always fall in the set. Strictly speaking, 10.3.5-6 is not an existing package version either, you won't find an pki-base-python2-10.3.5-6.rpm anywhere.

@tiran
Copy link
Member

tiran commented Jan 18, 2017

You would still depend on potentially non-existing package. pki-base-python2 was introduced in 10.3. pki-base will switch to Python 3 as soon as RHEL has Python 3 in its base distribution.

@HonzaCholasta
Copy link
Contributor

I see, didn't notice that. In this case, IMO either the current pki-base >= 10.2.1 or an unversioned pki-base-python2 is fine.

@tkrizek
Copy link
Contributor

tkrizek commented Jan 18, 2017

@HonzaCholasta Perhaps it's more of a personal preference, but I'd rather see an existing version of a certain package. Since the spec file is processed automatically, I guess it doesn't make a difference. But it could confuse someone who reads the file and looks for a certain version of the mentioned package.

pki-base provides pki-base-python2, but we should depend directly on
pki-base-python2 because in future pki-base may provide pki-base-python3
instead.

Source: cheimes@redhat.com

https://fedorahosted.org/freeipa/ticket/4985
@MartinBasti
Copy link
Contributor Author

bump for review

@HonzaCholasta
Copy link
Contributor

LGTM

@HonzaCholasta HonzaCholasta added the ack Pull Request approved, can be merged label Feb 7, 2017
@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Feb 7, 2017
@MartinBasti MartinBasti closed this Feb 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants