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

Second attempt at patches for an OpenBSD port #585

Merged
merged 2 commits into from Feb 7, 2018

Conversation

2 participants
@haqistan
Copy link
Contributor

haqistan commented Feb 5, 2018

… this time based off of 1.2 and after feedback from the upstream and other contributors at
#489

Instead of the "unixy" stuff I just add 'BSD' as a possible return value for
get_platform() and use it in several places. The idea of collapsing everything
down to Darwin/Windows/Unixy doesn't quite work out because we need to
distinguish between Linux and BSD in one place, so I thought this way of doing
it made it read more naturally. I also tried to deal with an issue in setup.py
in a way that might be acceptable to you.

Cheers, -A

Second attempt at patches for an OpenBSD port, this time based off of…
… 1.2

and after feedback from the upstream and other contributors at
#489

@haqistan haqistan requested a review from micahflee as a code owner Feb 5, 2018

@@ -316,7 +316,8 @@ def connect(self, settings=False, config=False, tor_status_update_func=None):
# guessing the socket file name next
if not found_tor:
try:
if self.system == 'Linux':
plat = common.get_platform()
if plat == 'Linux' or plat == 'BSD':
socket_file_path = '/run/user/{}/Tor/control.socket'.format(os.geteuid())

This comment has been minimized.

@mig5

mig5 Feb 5, 2018

Contributor

A minor nitpick - nothing wrong here - but rather than initialise a new variable 'plat' here, perhaps it would be better to be consistent and change line 134 of onion.py self.system = platform.system() to self.system == common.get_platform() ?

And then you can just make this if self.system == 'Linux' or self.system == 'BSD'?

What do you think?

Update patch as per the suggestion by @mig5 on
#585
Tested on current snapshot, both gui and cli work
@haqistan

This comment has been minimized.

Copy link
Contributor

haqistan commented Feb 6, 2018

I've updated my branch as per @mig5's suggestion - ditch new var, use self.system instead.

@mig5

This comment has been minimized.

Copy link
Contributor

mig5 commented Feb 7, 2018

This worked for me on FreeBSD at least with the 'dev_scripts', so I'm happy to merge this in.

@mig5

mig5 approved these changes Feb 7, 2018

@mig5 mig5 merged commit d1ce17c into micahflee:develop Feb 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mig5 mig5 added this to the 1.3 milestone Feb 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment