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

[2019.2.1] Add pygit2 requirement #53974

Merged
merged 8 commits into from Aug 6, 2019

Conversation

@Ch3LL
Copy link
Contributor

commented Jul 23, 2019

No description provided.

@dwoz
dwoz approved these changes Jul 23, 2019
s0undt3ch and others added 4 commits May 14, 2019
@waynew
waynew approved these changes Jul 29, 2019
@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

This is likely a Bad Idea™️.

pygit2 has a number of non-Python dependencies, especially if you want SSH support. libffi, libssh, and libgit2 at minimum. Then there's the fact that pygit2 is developed alongside libgit2, and its minor releases must track with the version of libgit2 which is installed. Assuming you have installed libgit2, if a 0.24.x libgit2 release is what you have installed (because that was what was available from the upstream repos), and pip installs pygit2 0.28.2.... in the immortal words of Thumper, "you're gonna have a bad time".

One other thing to consider is that the wheels built for pygit2 may or may not have been built against libssh2 (I haven't tested them), and if they haven't then installing them would leave you with a pygit2 that can't do ssh authentication.

Also, are these "static" requirements files only being used for the test suite?

@Ch3LL

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Are you stating its a bad idea to use the pygit2 pip packages? Are you suggesting we only use system packages which is what we were doing previously in our test suite with our salt-jenkins states. All of the pygit2 tests are passing on this PR.

And yes these requirements files are only for tests. Without this change the pygit2 tests are not running.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

I just installed the wheel into a virtualenv. The good news is that the wheel bundles its own libgit2.so. That libgit2.so doesn't link against libssh2, but from the test results it seems that it's successfully authenticating via SSH, so it seems that they're maybe statically compiling libssh support into libgit2? If that's the case then this is probably fine. Didn't mean to needlessly alarm anyone but I've seen version differences between libgit2/pygit2 bite people before and that raised a big red flag for me.

@Ch3LL

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

thanks for pointing it out. Looks like we are okay for now, but something to be aware of in the future for the tests.

Ch3LL added 2 commits Jul 31, 2019
@dwoz dwoz merged commit e268b95 into saltstack:2019.2.1 Aug 6, 2019
23 of 24 checks passed
23 of 24 checks passed
ci/py3/centos7/tcp This commit cannot be built
Details
WIP Ready for review
Details
ci/docs This commit looks good
Details
ci/lint This commit looks good
Details
ci/py2/amazon2 This commit looks good
Details
ci/py2/centos6 This commit looks good
Details
ci/py2/centos7 This commit looks good
Details
ci/py2/centos7/tcp This commit looks good
Details
ci/py2/debian8 This commit looks good
Details
ci/py2/debian9 This commit looks good
Details
ci/py2/fedora29 This commit looks good
Details
ci/py2/ubuntu1604 This commit looks good
Details
ci/py2/ubuntu1604/tcp This commit looks good
Details
ci/py2/ubuntu1804 This commit looks good
Details
ci/py2/windows2016 This commit looks good
Details
ci/py3/amazon2 This commit looks good
Details
ci/py3/centos7 This commit looks good
Details
ci/py3/debian8 This commit looks good
Details
ci/py3/debian9 This commit looks good
Details
ci/py3/fedora29 This commit looks good
Details
ci/py3/ubuntu1604 This commit looks good
Details
ci/py3/ubuntu1604/tcp This commit looks good
Details
ci/py3/ubuntu1804 This commit looks good
Details
ci/py3/windows2016 This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.