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.

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

Feature/psiphon dev #410

Closed
wants to merge 4 commits into from
Closed

Feature/psiphon dev #410

wants to merge 4 commits into from

Conversation

juga0
Copy link

@juga0 juga0 commented Sep 20, 2015

Not ready to merge. Look FIXMEs in psiphon.py, specially lines 68 and 122.
Thanks.

juga0 added 3 commits September 20, 2015 21:09
initial psiphon test. Failure cases doesn't fail propertly
4ca7eb3
add psiphon_install.sh, code should be improved
40f37a8
change the way HTTP request errors are handled
98bfa18 
* eliminate addFailureToReport because the errors are handled by HTTPTest
errback and addToReport
* add ConnectionDone exception to handleAllErrors, so that an HTTP request
to a domain that is not reachable is reported
@juga0
Copy link
Author

juga0 commented Sep 24, 2015

One FIXME less, line numbers in previous comment doesn't match anymore.

bassosimone
bassosimone reviewed Oct 8, 2015
View reviewed changes
ooni/errors.py
@@ -6,7 +6,7 @@
from twisted.internet.error import ConnectionRefusedError, TCPTimedOutError
from twisted.internet.error import DNSLookupError, ConnectError, ConnectionLost
from twisted.internet.error import TimeoutError as GenericTimeoutError
from twisted.internet.error import ProcessDone
from twisted.internet.error import ProcessDone, ConnectionDone
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ConnectionDone indicates the case where you make an HTTP request to a domain that is not reachable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Responding to myself, this probably indicates that the proxy closed the connection.

Copy link
Author

Choose a reason for hiding this comment

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

Exactly

@bassosimone
Copy link
Contributor

I am currently running into trouble because, when I run the psiphon test from inside the virtualenv, apparently pexpect is not found (even though it is installed in the virtualenv). This is probably my fault, will check again tomorrow.

In any case, for the records, this is the relevant part of the output:

[D] STDOUT: Traceback (most recent call last):\r\n File "/var/folders/bl/r84rmchd6c5848k6gxvvky_m0000gn/T/tmpBLbQCY", line 2, in \r\n from psi_client import connect\r\n File "$HOME/psiphon-circumvention-system/pyclient/psi_client.py", line 22, in \r\n from psi_ssh_connection import SSHConnection, OSSHConnection\r\n File "$HOME/psiphon-circumvention-system/pyclient/psi_ssh_connection.py", line 21, in \r\n import pexpect\r\nImportError: No module named pexpect\r\n

@juga0
Copy link
Author

juga0 commented Oct 8, 2015

Briefly: pip install pexpect
I'll comment more on why this is not in the script and all the other comments later.
Thanks a lot for all the detailed comments

@bassosimone bassosimone mentioned this pull request Oct 9, 2015
Merged
@bassosimone
Copy link
Contributor

@juga0: actually pexpect is installed both in the virtualenv and system-wide:

$ find venv -type d -name pexpect
venv/lib/python2.7/site-packages/pexpect
$ find /usr/local/ -type d -name pexpect
/usr/local//lib/python2.7/site-packages/IPython/external/pexpect
/usr/local//lib/python2.7/site-packages/pexpect

Must be something else.

@bassosimone
Copy link
Contributor

@juga0: sorted out the issue: I had more than one Python installed (the system one and the one installed with brew). Unfortunately /usr/bin/env picked up the wrong Python for which pexpect was not installed. Fixed by using sys.executable to execute the temp file, thus using same Python used to run ooniprobe.

@bassosimone
Copy link
Contributor

@juga0: Further updated my diff to use sys.executable to avoid removing my Python executable :).

bassosimone
bassosimone reviewed Oct 10, 2015
View reviewed changes
scripts/psiphon_install.sh
cp psi_client.dat $PSIPHON_PYCLIENT_PATH

# assuming to be inside a virtualenv
pip install jsonpickle pexpect
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to check whether you are inside a virtualenv?

Copy link
Author

Choose a reason for hiding this comment

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

hasattr(sys, 'real_prefix'). Inside a virtualenv real_prefix points to the prefix of the system Python, outside a virtualenv real_prefix should not exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

good to know!

@anadahz
Copy link

anadahz commented Oct 28, 2015

@juga0 is this ready to be tested?

juga0 referenced this pull request in juga0/ooni-probe Nov 5, 2015
improvements commented in https://github.com/TheTorProject/ooni-probe…
9f30c50 
…/pull/410
@hellais
Copy link
Member

hellais commented Nov 8, 2015

Can we merge this? @juga0 @bassosimone

@bassosimone
Copy link
Contributor

@hellais not yet. There is a problem that the test hangs when this test is run using -ng options but have not had time to understand this issue yet.

@bassosimone bassosimone self-assigned this Nov 9, 2015
Minor improvements commented on https://github.com/TheTorProject/ooni…
dd3c45f 
…-probe/pull/410

* add comment about why using usePTY
* add comment about why is needed to run psiphon from an script
* fix identation
@juga0
Copy link
Author

juga0 commented Nov 11, 2015

Improvements to psiphon install script in https://github.com/juga0/ooni-probe/commits/feature/psiphon-dev-install-script.
Probably that branch should be merged here

@juga0
Copy link
Author

juga0 commented Nov 11, 2015

Psiphon test specifications here ooni/spec#44

bassosimone referenced this pull request Nov 15, 2015
@bassosimone
Minor improvements commented on https://github.com/TheTorProject/ooni…
5393492 
…-probe/pull/410

* add comment about why using usePTY
* add comment about why is needed to run psiphon from an script
* fix identation
@bassosimone bassosimone mentioned this pull request Nov 15, 2015
Merged
@bassosimone
Copy link
Contributor

Merged #425 that was based on this pull request. I have resolved all the issues highlighted in this pull request in a quick-and-dirty way because it was imperative to merge a psiphon MVP into master.

hellais referenced this pull request Nov 20, 2015
@hellais
Merge branch 'master' of github.com:TheTorProject/ooni-probe
4d2804d 
* 'master' of github.com:TheTorProject/ooni-probe: (67 commits)
  Downgrading Twisted version is not needed
  Remove duplicate package
  Remove unneeded space
  Add IRC travis build notifications
  Add all supported twisted version, revise build dependencies
  Skip failing tests
  Proper fix for issue reported by @anadahz in #401
  psiphon: fix bug that psiphon hangs ooniprobe -ng
  Remove script to install psiphon
  psiphon.py: prepare for merge with master
  Minor improvements commented on https://github.com/TheTorProject/ooni-probe/pull/410
  psiphon: use the python provided by sys.executable
  change the way HTTP request errors are handled
  add psiphon_install.sh, code should be improved
  initial psiphon test. Failure cases doesn't fail propertly
  Properly add BTC address
  Update README.rst
  Update README.rst
  deal with existing, but empty, reporting.yml
  Fix misleading error message
  ...
hellais referenced this pull request Feb 5, 2016
@hellais
Minor improvements commented on https://github.com/TheTorProject/ooni…
9921f48 
…-probe/pull/410

* add comment about why using usePTY
* add comment about why is needed to run psiphon from an script
* fix identation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bassosimone bassosimone

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@juga0 @bassosimone @anadahz @hellais