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

Remove port bind pre-check #882

Closed
wants to merge 2 commits into from
Closed

Conversation

BKreisel
Copy link
Contributor

@BKreisel BKreisel commented Oct 3, 2015

Removes port pre-bind check to address OS X permissions issue #680. This also removes the psutil dependency from the project.

Appropriate code to check for exceptions exists at bind time and from what I can see, removing the following lines will not introduce undefined behavior

@jmhodges
Copy link
Contributor

jmhodges commented Oct 3, 2015

Is the Popen executable="/bin/bash" change happening in another PR?

@BKreisel
Copy link
Contributor Author

BKreisel commented Oct 3, 2015

@jmhodges. Yea, i'm wrapping it up with all the other integration changes I did. I seperated this one out because it was more "controversial" 🎁

@kuba
Copy link
Contributor

kuba commented Oct 3, 2015

Does psutil not work in Mac OS X at all? What's the deal here? Could we fix it other way rather than removing totally? I don't see any alternatives discussed in #680.

I kind of liked @schoen's idea to check running processes and report back to the user which one is actually using the intended port ("hey, you have Apache running, please shut it down" is a big win for UX). NB It might not necessarily happen before bind - could do the check in exception handling phase. However, I do have to mention that the current implementation is not ideal either, c.f #255.

If you add/remove any dependency (psutil here) you have to update setup.py. You should also inform Debian devs to adjust their deps list (either cc: @fmarier, @hlieberman here, or report/submit PR at https://github.com/letsencrypt/letsencrypt-debian).

@BKreisel
Copy link
Contributor Author

BKreisel commented Oct 3, 2015

@kuba: Leaving psutil in just means that the integration tests on OS X are going to have to be executed with root privs.

The UX argument makes sense to me. I'm cool with leaving it in and closing this pull if that is the consensus.

@kuba
Copy link
Contributor

kuba commented Oct 3, 2015

How about we catch AccessDenied exceptions and ignore them silently (logger.info + inline comment about Mac incompat)! ;)

@BKreisel BKreisel closed this Oct 3, 2015
@kuba
Copy link
Contributor

kuba commented Oct 4, 2015

FTR the above idea is exploited in #886.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants