Skip to content

pythonPackages.pywinrm: 0.1.1 -> 0.2.2 #26161

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 2, 2017
Merged

pythonPackages.pywinrm: 0.1.1 -> 0.2.2 #26161

merged 3 commits into from
Jun 2, 2017
+104 −19

Conversation

elasticdog
Copy link
Contributor

Motivation for this change

Bump version to latest upstream release and remove the already broken/optional Kerberos support.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

The latest version of pywinrm has a new dependency on requests_ntlm which requires the lower-level ntlm-auth. I've removed the optional kerberos dependency, as it was already broken, now requires additional dependencies to function, and I couldn't figure out a clean way of manipulating the LD_LIBRARY_PATH (or patch things with the absolute path) to fix the original error:

error: libgssapi_krb5.so: cannot open shared object file: No such file or directory

The other changed dependencies were just making things accurate with the upstream requirements. Here's the pipdeptree output, if you're curious:

pywinrm==0.2.2
  - requests [required: >=2.9.1, installed: 2.14.2]
  - requests-ntlm [required: >=0.3.0, installed: 1.0.0]
    - ntlm-auth [required: >=1.0.2, installed: 1.0.3]
      - six [required: Any, installed: 1.10.0]
    - requests [required: >=2.0.0, installed: 2.14.2]
  - six [required: Any, installed: 1.10.0]
  - xmltodict [required: Any, installed: 0.11.0]

The tests I disabled require a local IIS server and network access.

Sorry, something went wrong.

@mention-bot
Copy link

@elasticdog, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh to be a potential reviewer.

FRidh
FRidh requested changes May 28, 2017
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Please see the header of the file and move the expressions.

pkgs/top-level/python-packages.nix Outdated
sha256 = "12cwv0ywlfvspnpjlc49yccnzp7ijvjwrzhl31wzb0y4kqgzzxx0";
};

buildInputs = with self; [ mock pytest unittest2 ];
Copy link
Member

Choose a reason for hiding this comment

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

checkInputs

pkgs/top-level/python-packages.nix Outdated
};

propagatedBuildInputs = with self; [ isodate kerberos xmltodict ];
buildInputs = with self; [ mock pytest ];
Copy link
Member

Choose a reason for hiding this comment

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

checkInputs

@FRidh
Copy link
Member

FRidh commented May 28, 2017

The latest version of pywinrm has a new dependency on requests_ntlm which requires the lower-level ntlm-auth. I've removed the optional kerberos dependency, as it was already broken, now requires additional dependencies to function, and I couldn't figure out a clean way of manipulating the LD_LIBRARY_PATH (or patch things with the absolute path) to fix the original error:

Typically it tries to load the library somewhere with ctypes or cffi. In that function, don't let it use the name of the library but instead pass in the absolute path to the library.

@elasticdog elasticdog force-pushed the pywinrm branch 2 times, most recently from e5b79fe to d4553a1 Compare May 28, 2017 18:19
@elasticdog
Copy link
Contributor Author

Thank you for the feedback! I force pushed the branch after added the following changes:

  • moved the three libraries into dedicated files under pkgs/development/python-modules/
  • switched to using the proper checkInputs
  • instead of fetchurl, refactored to use fetchpypi when possible and fetchFromGitHub for ntlm-auth since they strip out the tests directory in the PyPI tarball

@pSub pSub added the 8.has: package (update) This PR updates a package to a newer version label May 29, 2017
@elasticdog
Copy link
Contributor Author

@FRidh were you wanting any other changes here? Everything is working properly for me when using it locally.

I don't have any real experience with the Kerberos ecosystem, and I did try to trigger the runtime error with the old pywinrm package version (which is why it got labeled as broken) by following the basic instructions laid out in diyan/pywinrm#21, but couldn't reproduce the problem. If you want, I can try to add back in the kerberos support as an optional install and package up the new requests-kerberos dependency, but again, I'm not confident that I can test it properly to know if it's working.

FRidh
FRidh approved these changes Jun 2, 2017
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Haven't tested it but looks good to me.

@FRidh FRidh merged commit c443fcf into NixOS:master Jun 2, 2017
@elasticdog elasticdog deleted the pywinrm branch June 2, 2017 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (update) This PR updates a package to a newer version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@elasticdog @mention-bot @FRidh @pSub