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

tests: remove python_dependencies for smbserver from our tree #5094

Closed
wants to merge 3 commits into from

Conversation

@mback2k
Copy link
Member

mback2k commented Mar 13, 2020

Users of the SMB tests will have to install impacket manually.

Reasoning: our in-tree version of impacket was quite outdated
and only compatible with Python 2 which is already end-of-life.
Upgrading to Python 3 and a compatible impacket version would
require to import additional Python-only and CPython-extension
dependencies. This would have hindered portability enormously.

See also #5085 for the previous discussion leading into this PR.

@mback2k

This comment has been minimized.

Copy link
Member Author

mback2k commented Mar 13, 2020

This PR will only covers the removal of our in-tree Python dependencies. For Python 3 another PR will follow.

@mback2k mback2k self-assigned this Mar 13, 2020
@mback2k mback2k added the tests label Mar 13, 2020
@dfandrich

This comment has been minimized.

Copy link
Collaborator

dfandrich commented Mar 13, 2020

@mback2k

This comment was marked as outdated.

Copy link
Member Author

mback2k commented Mar 13, 2020

@dfandrich thanks, would you mind pushing the relevant change as an additional commit to my branch? You should have permission for that.

@mback2k

This comment has been minimized.

Copy link
Member Author

mback2k commented Mar 13, 2020

@bagder test 323 is now failing in this PR, because I made stunnel available to the CI builds. But I do not have any clue why it is failing, would you mind taking a look at the failed Azure jobs?

@mback2k mback2k force-pushed the mback2k:remove-python-deps branch from 5453237 to c44bc7d Mar 13, 2020
@mback2k

This comment has been minimized.

Copy link
Member Author

mback2k commented Mar 13, 2020

@dfandrich for now I added py27-impacket to the CirrusCI builds due to smbserver.py not (yet) being fully Python 3 compatible logic-wise. I will work on that in a follow-up PR.

@mback2k mback2k force-pushed the mback2k:remove-python-deps branch from c44bc7d to 86f6c73 Mar 13, 2020
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Mar 14, 2020

I don't see any azure jobs at all?

@mback2k

This comment has been minimized.

Copy link
Member Author

mback2k commented Mar 14, 2020

I don't see any azure jobs at all?

@bagder that is what I meant with "something is wrong with Azure CI not triggering for PRs anymore". After doing a force-push they weren't triggering anymore for some unknown reason.

@mback2k mback2k force-pushed the mback2k:remove-python-deps branch 3 times, most recently from 84e6f9e to 8ac64cd Mar 14, 2020
@mback2k

This comment was marked as outdated.

Copy link
Member Author

mback2k commented Mar 14, 2020

@dfandrich would you mind taking a look at the CirrusCI failures? I do not know what is wrong with those packages.

@mback2k mback2k marked this pull request as ready for review Mar 14, 2020
@mback2k mback2k requested review from bagder and dfandrich Mar 14, 2020
@mback2k

This comment has been minimized.

Copy link
Member Author

mback2k commented Mar 14, 2020

Azure CI builds are for unknown reasons missing for this PR, but are looking good on my end.

mback2k added 2 commits Mar 14, 2020
Users of the SMB tests will have to install impacket manually.

Reasoning: our in-tree version of impacket was quite outdated
and only compatible with Python 2 which is already end-of-life.
Upgrading to Python 3 and a compatible impacket version would
require to import additional Python-only and CPython-extension
dependencies. This would have hindered portability enormously.
@mback2k mback2k force-pushed the mback2k:remove-python-deps branch from 5e56138 to 79aa91e Mar 14, 2020
@mback2k mback2k removed the request for review from dfandrich Mar 14, 2020
@mback2k mback2k force-pushed the mback2k:remove-python-deps branch from 79aa91e to c5095fb Mar 14, 2020
@mback2k

This comment has been minimized.

Copy link
Member Author

mback2k commented Mar 14, 2020

@dfandrich I will just add impacket to the FreeBSD builds in another PR to have this finished.

@mback2k mback2k requested review from MarcelRaad and removed request for bagder Mar 14, 2020
@mback2k

This comment has been minimized.

Copy link
Member Author

mback2k commented Mar 14, 2020

Pending topics for follow up PRs:

  • Add impacket to FreeBSD CirrusCI builds.
  • Fully port smbserver.py to Python 3.
Copy link
Member

MarcelRaad left a comment

Also AppVeyor would be a candidate to install impacket later. We currently don't install any additional software there, but test 1451 is currently run and passing.

@mback2k

This comment has been minimized.

Copy link
Member Author

mback2k commented Mar 15, 2020

@MarcelRaad good idea, I guess it will make sense to include it in the WSL-based builds I am creating.

For the other builds I am not sure how to install something in the Git bash.exe environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.