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
Adjustments for setup requirements #255
Conversation
9111930
to
e2b55ae
Compare
e2b55ae
to
2b9c49f
Compare
| 'dnspython': 'dnspython >= 1.11.1', | ||
| 'gssapi': 'gssapi > 1.1.2', | ||
| 'gssapi': 'gssapi >= 1.2.0', |
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.
Why did you raised required 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.
Both gssapi and cryptography has lower version in our specfile. I don't want to have inconsistent versions between RPM and pip installs
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 want to be consistent between RPM and pip, therefore I require the same versions as are available in Fedora 24:
$ rpm -qa python-gssapi python2-cryptography
python2-cryptography-1.5.3-3.fc24.x86_64
python-gssapi-1.2.0-1.fc24.x86_64
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.
Why should we care about F24 versions? We have minimal required versions specified in specfile and these should be used as minimal versions for pip too.
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.
You are treating PyPI packages like you are treating RPM packages. That is not how it works. Distribution packages have curated, well test combinations of package versions. Do you have enough resources to test the packages with every release of python-cryptography since 0.9?
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 agree with Martin. The spec file contains the actual minimal required versions of the packages that IPA is known to work with and you can track down in git history why exactly it is these particular versions. So NACK as well.
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.
Did you verify that FreeIPA works with cryptography 0.9?
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.
AFAIK we had a bug somewhere that 0.9 and older versions break Custodia actually.
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.
No, but in case you know that cryptography 0.9 doesn't work, you should open a ticket and this should be fixed on both levels, spacfile and setup.py, not just in the one half.
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.
You are still applying RPM packaging rules to PyPI packages.
| 'pyldap': 'pyldap >= 2.4.15', | ||
| 'pytest': 'pytest < 3.0', |
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.
Why pytest < 3 ? in specfile we have >=2.6. If pytest > 3 does not work with IPA, a ticket should be opened and specfile fixed as well.
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.
Same here
I couldn't run any test with pytest 3. With 2.x some tests were working out of the box.
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.
Please open a ticket
|
I found some changes in versions of dependencies I don't like, because there is no explanation why it is needed and it is out of sync between specfile and setup.py |
2b9c49f
to
2550f3a
Compare
|
Better now, but commit message missing explanation why bumping requires was needed. |
2550f3a
to
1a8bf57
Compare
|
The commit message doesn't explain why python-gssapi version is raised. Is it required by something? It also doesn't explain if the minimal required version of python-cryptography should be 1.3. Review would be much smoother if this information was here since the beginning. That said, answers to those questions are not important. Fedora 23+, RHEL 7.3, PyPi all have the same or newer versions and it is actually more work to install older versions. (Not sure about Debian) Or it might not even be possible. So there is no point to waste time with discussing why it needs to be bumped. If it was a version which would not be on target platforms then it would be different story. @mbasti-rh is there any other reason for having full explanation of version bumps which I don't see? If not can we move the review forward to unblock #263 ? |
|
@tiran You can split patch to useful part and please send unneeded bumping of requires as separate pull request, we can continue with discussion there about bumping versions. It is unrelated part of patch and should be in separated commit anyway. |
|
@mbasti-rh The bumped version numbers are required. gssapi needs to be bumped because 1.1.x has wrong dependency information for Python 3 (enum34). cryptography 0.9 does not build any more. gssapi 1.2 and cryptography 1.3 are the oldest releases that are actually been tested by QE. I did not bother to verify older releases because I consider it a waste of time and resources. In a couple of weeks we have to bump up cryptography to 1.7 anyway. |
|
PS: There is no technical reason to bump the version of python-gssapi in freeipa.spec. The enum34 dependency issues is solely a Python packaging bug. It does not affect RPM packages. Since you insist on syncing PyPI versions with RPM versions, I had to bump both. Have it your way. |
I don't see reason why bumping requires just because we are unable to build on fedora. Fedora is not the only linux distro. |
So finally we have reason to bump version, which should be docummented in git history as separate commit. |
|
You said Fedora, I didn't. The build bug is not related to Fedora at all. Cryptography 0.9 does not build on any distribution or platform with a recent version of OpenSSL. Touché, I said Fedora in the commit message. |
|
So create a separate commits:
I'm still not persuaded with need for bumping cryptography. |
|
Would you rather claim to be compatible with a broken, unsupported, and old version? |
7f0ea93
to
d2936b3
Compare
|
Well from our (as upstream) POV 0.9 and later is required for Custodia to work correctly. This requirement was introduced by me in commit aa74995 when I was building 4.3 in Copr for CentOS 7. There was ye olde 0.8 something version and I found empirically that 0.9 or later is required for replica promotion to work (at that time 1.2.1 was the most up-to-date version built in Brew IIRC). Yes, this version is ancient and vast majority of distros does not support it anymore but then it is their job to provide newer version fullfilling our Required and I see no point in artificially bumping it in upstream unless some of our code depends on functionality of newer version. I mentioned the CentOS story as an example that demonstrates that you never know on what distro your software is being ported. That said, if you are afraid that it can break the PIP use-case then I am fine with bumping the version but as @mbasti-rh said, please split version bumps into a separate commit with clean explanation of the reasons (already provided in the commit message). This makes it easier for our future selves to review the build/runtime requirements during spec file cleanups and similar work. I remember that @jcholast was very frustrated when he was cleaning up BuildRequires recently and was unable to find any reasonable explanation for many of them in git history. |
|
@martbab Welcome to the party! This discussion has been running for a very long time and in multiple places. Let me bring you up to speed. First of all the requirements in Next up a version information like "cryptography >= 0.9" means that any version equal or greater than 0.9 is known to work. If you follow upstream development of OpenSSL and Cryptography closely then you are aware that any version of cryptography < 1.3 does no longer compile against a recent version of OpenSSL 1.0.2. CFFI bindings are very sensitive to subtle changes in the ABI and C API. OpenSSL tend to break both every now and then. Finally this discussion is pointless. I will bump the version requirements of cryptography to 1.7.0 in a matter of weeks. BZ for RHEL has been filed. The version 1.7.0 hasn't been released yet. it will contain two important fixes (lock and osrandom) and a new feature for @frasertweedale (multi RDN). 1.2 |
|
As I said, if 0.9 break your PyPI work feel freee to bump it but please split the version bumps into a separate commit on top of ipasetup fixes. |
* Fix some typos, missing or surplus dependencies. * Remove setup requirement on wheel since it triggers download. ipatests is now installable. Tests need further changes to be runable. https://fedorahosted.org/freeipa/ticket/6468 Signed-off-by: Christian Heimes <cheimes@redhat.com>
d2936b3
to
785f924
Compare
|
Thank you. It seems that 'bdist_wheel' target is broken in your PR: Do i need some of your other pull-requests to build wheels or this is a genuine issue? |
|
The bdist_wheel command requires the Python wheel package installed in the system. Since setup.py no longer contains Does it makes sense to include the dependency in freeipa.spec as build requirement? Technically it's not a build requirement for RPMs. |
|
Installing python-wheel worked, thanks. I have discovered some other missing dependencies in minimal Docker container. I will investigate them some more and open a ticket. I think there is no need to add python-wheel to BuildRequires now. |
|
Fixed upstream |
|
@martbab The wheel bundle and packages need some documentation. I have started some docs but they are not finished.. |
Fix some typos, missing or surplus dependencies. ipatests is now
installable. Tests need further changes to be runable.
https://fedorahosted.org/freeipa/ticket/6468
Signed-off-by: Christian Heimes cheimes@redhat.com