BwAuthority can't be run out of the box without manual work
Currently the bwauthority is quite out of date with the current python ecosystem and doesn't play nice in a dev environment.
The submitted patch makes the following updates:
- Removes the cron setup to a separate script (so running setup.sh doesn't alter your crontab)
- Changes the shell script to use a modern pip instead of peep (peep is deprecated and hash checking is incorporated into pip as of version 8.0 (https://pypi.python.org/pypi/peep/)
- Updated dependencies as far as possible (Elixer's last version is 0.7.1 and is only compatible with SQLAlchemy <= 0.7.8, obvious future enhancement to move away from Elixir)
- Added a convenience script to kill all the spun up processes
- Updated the ReadMe and the .gitignore to reflect these changes.
Trac:
Username: davidwf
Activity
Trac:
Username: davidwf-
Gzipped patch attached + github branch here: https://github.com/davidfaulkner12/bwauth/tree/nicedev
Trac:
Username: davidwf 
This seems good, but I'm wondering if it conflicts or subsumes some outstanding work in #20466 (moved), #20452 (moved), #20454 (moved), #20452 (moved)
Trac:
Cc: chelseakomlo, mikeperry, teor, AgentSeb, davidwf to chelseakomlo, mikeperry, teor, AgentSeb, davidwf, tom
Reviewer: chelseakomlo, tjr to chelseakomlo, tomThis seems good to me as well (I tested this on macOS).
If virtualenv is used, #20466 (moved) shouldn't be an issue (I believe).
#20454 (moved) should also be addressed with this patch.
davidwf: thanks for this patch!
In general:
Why did you downgrade from SQLAlchemy 0.7.10 to 0.7.8? Whichever way you choose to go, please find all instances of SQLAlchemy-0.7.* and make them consistent.
For future patches, it is ok to provide a branch and use multiple commits - in fact, we prefer it!
In particular, it is nice if whitespace changes are in a separate commit to functional changes.
In particular:
In setup.sh and setup_cron.sh:
readlink -f works with gnu readlink, but not with BSD readlink. It is possible to install BSD readlink on Linux, and GNU readlink on BSD or macOS. So we can't depend on uname, we should just try basename if readlink doesn't work here:
+# readlink -f does not work on Mac +if [ `uname` != "Darwin" ] +then + SCANNER_DIR=$(readlink -f "$SCANNER_DIR") +fiAlso, let's use the same code in all the scripts to do this, for consistency.
Some OSs only have a non-versioned python (e.g. macOS), so we should fall back to "which python" here:
+PYTHON=$(which python2.7 || which python2.6)In stop_scan.sh:
This is a dangerous strategy if the process has already died, and its PID is being used by something else. Can we check the process has the right name in a cross-platform way?
kill -9 `head -1 $PIDFILE` && rm $PIDFILEYou may also be better making the parent scanner script Tor's __OwningControllerProcess, so tor exits automatically when the script exits. But you'll just have to kill the bwauthority child processes.
Please don't copy environmental variables like this:
+SCANNERS_PER_TOR_COUNT=4 +TOR_COUNT=2 +SCANNER_COUNT=$(($SCANNERS_PER_TOR_COUNT * $TOR_COUNT + 1))It makes it easy for them to get out of sync with the launch script. Also, if someone changes the variables, then stops the scanners, some will be left over. Instead, why not do:
for PIDFILE in ./data/scanner.*/bwauthority.pidand the same for the tor instances.
Trac:
Status: merge_ready to needs_revision-
Please see changes at 'git@github.com:davidfaulkner12/bwauth.git', branch 'nicedev'.
Why did you downgrade from SQLAlchemy 0.7.10 to 0.7.8? Whichever way you choose to go, please find all instances of SQLAlchemy-0.7.* and make them consistent.
Updated -- the PYTHONPATH variables in the shell scripts had no impact (and didn't resolve to anything on my machine at least), so I removed them. I used 0.7.8 because that's the last documented "compatible" version with Elixer.
For future patches, it is ok to provide a branch and use multiple commits - in fact, we prefer it!
Noted! It was actually always in Github, I just through patches were preferred. :-)
readlink -f works with gnu readlink, but not with BSD readlink. It is possible to install BSD readlink on Linux, and GNU readlink on BSD or macOS. So we can't depend on uname, we should just try basename if readlink doesn't work here:
I checked to see if the command fails, and only set the variable if necessary. This should work!
Some OSs only have a non-versioned python (e.g. macOS), so we should fall back to "which python" here:
Done.
In stop_scan.sh: ...
stop_scan.sh is actually just copy and pasted from the first lines of the run_scan.sh. I don't disagree with anything you said, but I'm not confident that I can make those changes in a way that works reliably without digging too deeply past the initial scope of "get this working on a laptop".
Trac:
Username: davidwf Replying to davidwf:
In stop_scan.sh: ...
stop_scan.sh is actually just copy and pasted from the first lines of the run_scan.sh. I don't disagree with anything you said, but I'm not confident that I can make those changes in a way that works reliably without digging too deeply past the initial scope of "get this working on a laptop".
If we don't want to introduce tech debt in multiple places, stop_scan.sh doesn't need to be added as part of this patch, but I agree having this as a convenience would be useful.
-
When I lost the bwauth box that my bwauth was runnign on, I lost the ability to keep working on this. Now that I have a new box I can resume the bwauth queue of patches. Feel free to bug me ~weekly about them.
I'm going to try running a second bwauth (on the same box), see if that causes any abnormalities in the existing one, compare results, and then start testing changes...
-
Hm, I think we want to solve the following:
- setup_cron.sh needs to marked executable
- We should centralize the variables and functionality shared in run_scan and stop_scan
- I don't know why, but the setup_cron.sh script wipes out any existing cron entries you have. Or at least that is my experience.
I can probably take these as actions to resolve and then we can merge.
Single tickets that deal with multiple issues are really hard to resolve. Please split up any issues that you'd like to continue working on into their own tickets, and make this ticket the parent.
- Removes the cron setup to a separate script (so running setup.sh doesn't alter your crontab)
wontfix - In #24088 (moved), we want to get rid of setup.sh, it's just too fragile. We need to write good instructions instead.
- Changes the shell script to use a modern pip instead of peep (peep is deprecated and hash checking is incorporated into pip as of version 8.0 (https://pypi.python.org/pypi/peep/)
revise - this needs to become a change to the requirements file, see #24089 (moved)
- Updated dependencies as far as possible (Elixer's last version is 0.7.1 and is only compatible with SQLAlchemy <= 0.7.8, obvious future enhancement to move away from Elixir)
split off into its own ticket, please
- Added a convenience script to kill all the spun up processes
split off into its own ticket, please
- Updated the ReadMe and the .gitignore to reflect these changes.
put these in the branches for the relevant ticket(s), please
Trac:
Status: needs_review to needs_revisionmentioned in issue #24089 (moved)