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

Jump to bottom

tests: remove python_dependencies for smbserver from our tree #5094

Closed
wants to merge 3 commits into from
Closed

tests: remove python_dependencies for smbserver from our tree #5094

wants to merge 3 commits into from

Conversation

mback2k
Copy link
Member

@mback2k 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
Copy link
Member Author

mback2k commented Mar 13, 2020 •
edited

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
Copy link
Contributor

dfandrich commented Mar 13, 2020 via email

@mback2k

This comment has been minimized.

Sign in to view
@mback2k
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
Copy link
Member Author

mback2k commented Mar 13, 2020 •
edited

@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.

@bagder
Copy link
Member

bagder commented Mar 14, 2020

I don't see any azure jobs at all?

@mback2k
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 remove-python-deps branch 3 times, most recently from 84e6f9e to 8ac64cd Compare March 14, 2020 18:35
@mback2k

This comment has been minimized.

Sign in to view
@mback2k mback2k marked this pull request as ready for review March 14, 2020 22:27
@mback2k
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
tests: remove python_dependencies for smbserver from our tree
406380d 
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
ci/tests: install test suite dependencies stunnel and impacket
6336bcd
@mback2k mback2k removed the request for review from dfandrich March 14, 2020 22:49
@mback2k
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 March 14, 2020 22:54
@mback2k
Copy link
Member Author

mback2k commented Mar 14, 2020 •
edited

Pending topics for follow up PRs:

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

MarcelRaad
MarcelRaad approved these changes Mar 15, 2020
View reviewed changes
Copy link
Member

@MarcelRaad MarcelRaad left a comment

Choose a reason for hiding this comment

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

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 mback2k closed this in 4be2560 Mar 15, 2020
@mback2k mback2k mentioned this pull request Mar 15, 2020
Closed
@mback2k
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.

This was referenced Mar 15, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcelRaad MarcelRaad MarcelRaad approved these changes

Assignees

@mback2k mback2k

Labels
tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mback2k @dfandrich @bagder @MarcelRaad