-
Notifications
You must be signed in to change notification settings - Fork 12
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
Pin dependencies from homebrew package, fix travis #57
Conversation
There seems to be a problem with the CI which is failing due to:
This should be fixed before we merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a request for information to better understand the diff :-)
I suggest, that if we would like to make a new release of OONI Probe legacy with these dependency changes, that the A good reference for that could be the macOS homebrew package: |
Quick and dirty onliner to extract the requirements.txt via this one-liner
|
Comparing #57 (comment) with |
Thanks for looking into it.
Should be
Small typo, correct: I will also compare with my install and try with these pinned dependencies. |
Cross checking in 2 different Linux installations.
This is a very low version, min encountered version:
This is a very low version, min encountered version:
Version: `2.2.0
This is a very low version, min encountered version:
This is a very low version, min encountered version:
Version:
Min encountered version Version:
Min encountered version Version:
Min encountered version: |
These linux installations you looked at had OONI Probe installed in which way? If these installs were done with pip (I am assuming that is the case), then the dependencies will just be a result of the point in time in which you did the installation so I am not really sure it's useful to do this check this way. My suggestion is we use the pinned versions in macOS (even if they are old, because they are tested by many active installs) and you check that with these versions it works without any problems on linux. |
These are 2 active ooniprobe installations that submit daily measurements and are not be working well.
I don't think that I can test this thoroughly. As you know we happen to find many issues much later in measurements. Can you confirm that you current macOS installation runs with said dependencies? |
@anadahz brew install ooniprobe just works for me, i.e., it gives me a working binary. |
Nice thanks, can you run a full ooniprobe cycle; all ooniprobe tests and check if there is anything that errors? Travis still errors, I will a last test with the same dependencies as in the homebrew version. |
In dc8d861 I do version pinning according to the homebrew package versions, however this commit errors on dependency issues:
@hellais Is homebrew doing a differente dependency check? |
You only pinned the direct dependencies. You need to pin both the direct and the indirect dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @anadahz for raising the issue and make this work again! For me, we can squash and merge!
Testing it on a lepidopter. |
@hellais I guess this error is not related to the version of the dependencies?
The package versions as installed in the base system:
|
@anadahz what do you want me to do to test it? I've manually run |
@sbs thanks! I think this suffices as a test, did you see any errors? I 'm also testing this on lepidopter and don't see any issues apart from the error mentioned in #57 (comment) . |
Nope. The usual situation that a Tor circuit was not okay and a request was retried. This happens some times when using OONI via Tor, but, apart from that, all looked like okay. |
@bassosimone Great, LGTM? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anadahz I noticed that we are pinning a specific version of Twisted. Yet, IIRC, the Travis script builds with many Twisted versions.
My understanding is that this has now become not so useful, because of the pinning. Do you agree? Can you perhaps also reduce the unnecessary builds in such case?
Pin ooniprobe dependencies: * Pin direct and indirect dependencies from a known working macOS homebrew package. https://github.com/Homebrew/homebrew-core/blob/master/Formula/ooniprobe.rb Travis: * Instruct Travis to manually fetch the gpg public key from deb.torproject.org as the archive signing key stored on the keyserver is subject to (CVE-2019-13050). * Remove docker as it's not currenlty * Switch to Ubuntu Xenial in Travis as Ubuntu Trusty is not a supported release by torproject Debian repository. * No need for multiple version tests, dependencies pinned to a specific version
@sbs The extra versions are currently not used (because of dependencies pinning). I removed them, thanks for checking out! |
@anadahz thanks for your work. I'll merge this tomorrow. |
Thanks for working on this. It may be useful as a checklist for testing to look at: https://github.com/ooni/spec/blob/master/attic/probe-legacy-release-procedure.md. I should also point out how with this fix (which I do think is the right thing to do) we may run into issues if we care to update OONI Probe pacakges (which Id on't think we do) since we are pinning the dependencies to a specific version which may not be the packaged version a particular distro. @anadahz As a heads up, I find it best to not force push when working on a PR that others are looking at. You can always squash at the end when merging, but it's useful to see the individual commits in the PR to know what changed since you last looked at it. In any case if you folks have done some testing of this on macOS, linux and lepidopter, I think it can be merged. |
@hellais thanks for reminding us the release procedure guidelines. From my side it's OK to be merged. |
@anadahz did you go through the testing checklist on linux and lepidopter: https://github.com/ooni/spec/blob/master/attic/probe-legacy-release-procedure.md#testing? |
@hellais Isn't this what Travis does? |
No travis only does the unit tests on a specific ubuntu version. Automated testing is not a full replacement for manual testing. |
I'm going to investigate a bit further the following error (mention in #57 (comment)) and see if I can fix it: |
It seems that pypcap vesion |
@anadahz How do I test this |
libdnet
libpcap
pip
manipulation/traceroute
All seems to be WAI. |
Pin werkzeug package. Pin 1.2.1 version of pypcap to resolve certain versions of *nix that report: No module named pcapy.
@sbs Thank you for posting the package versions. |
It’s still unclear to me what application level testing has been done on this branch. My suggestion would be to make a checklist of what has been tested WRT application functionality. For example it’s useful to confirm that the pypcap bug is fixed, that the tests that require scapy run as expected. |
@hellais Please follow the discussion on this PR (#57). TL;DR
|
Testing pull request #57Here you can find the tests used to verify that the python version of ooniprobe works as expected. Lepidopter issue reportThe issue has been first identified in A quick fix has been issued and the OP of the issue was able to run the python version of ooniprobe successfully. Exact details of the fix can be found here, here and here This pull request #57I opened a pull request to fix the issue identified in: TheTorProject/lepidopter#106 Fixing Travis issuesDue to an unrelated issue (CVE-2019-13050) Travis was failing so I issued a fix in: b4efb6a In summary the changes to Travis CI:
Working towards a uniform OONI Probe legacy releaseUpon discussion with @hellais we decided that is better to pin all dependencies so that we can minimize all errors that may introduced by new versions of the dependencies, the result upon multiple discussions and PR comments can be found here: b4efb6a Package pcapy bugDuring my tests with different package versions, I encountered another bug:
Steps taken to identify and fix the pcapy package bug:
The fix is to pin the version of pypcap to
The commit to the fix can been found in e1481e6 Testing ooniprobeDuring the bug fixing the following tests took place. Standalone tests
Deck tests
Running in production with lepidopterRunning the package versions defined in https://github.com/ooni/probe-legacy/pull/57/commits doesn't seem to bring any issues. |
@anadahz thanks for detailing what you did to test the app. This is useful!
Did you understand what was causing this issue with pypcap? If you have a way to reproduce the bug in a minimal way it would be useful to file an issue here too: http://github.com/pynetwork/pypcap/. I think this PR can now be merged. |
Specific (newer) versions of pypcap have introduced this (as it seems from #57 (comment)) *nix specific bug.
|
It's not clear to me from that comment that it's a *nix bug or even specific to that version of pypcap (@bassosimone is using pypcap 1.2.2 on macOS without any issues). What version of libpcap do you have on debian? I believe we dropped support for libpcap 1.0 in pypcap recently (see: pynetwork/pypcap#86), so that may be problem. |
I consider macOS to not be *nix and this is @bassosimone cannot reproduce the pcapy bug.
|
lol, ok....
I guess some more work on debugging this issue would be needed, but it's probably out of scope for this PR. |
Sure, feel free to mention me on an issue/ticket so that we can investigate it. @hellais Thanks for reviewing this PR. |
Why is that? |
Newest Twisted version breaks ooniprobe web GUI downgrading to a known
good version, related issues: