depend on phantomjs_bin to provide the phantomjs binaries #78
Conversation
…t needs to run in isolation
|
Tests run in isolation with this change. |
src/nti/contentrendering/phantom.py
Outdated
| @@ -27,6 +31,15 @@ | |||
|
|
|||
| logger = __import__('logging').getLogger(__name__) | |||
|
|
|||
| # Make sure our phantomjs executable is, in fact, user executable | |||
| # https://github.com/jayjiahua/phantomjs-bin-pip/issues/1 | |||
| st = os.stat(phantomjs_exe) | |||
There was a problem hiding this comment.
I suggest deferring all this until it is used (e.g., inside run_phantom_on_page), and only if running fails.
- Import-time side-effects are generally bad; and
- Especially when they can fail; and
- This may not be necessary at all, better to let the OS tell us if it is.
|
@jamadden out of curiosity is this code using I noticed warnings was imported when used. Should we do something similar with errno and stat and only import those in the except block if we fall down that case? |
…ermission denied error.
Yes.
Probably not.
I probably would, yes. I think initially the delayed import of But for really-unexpected-exceptions-that-almost-certainly-only-happen-once, like this one, there is something to be said for not polluting the namespace and keeping the global import list small (small import lists help readers and tools both). As soon as they were needed more than once, though, move them to the top. (There's no argument to be made for optimization, though, as many other dependencies as get pulled in.) |
src/nti/contentrendering/phantom.py
Outdated
| # Make sure our phantomjs executable is, in fact, user executable | ||
| # https://github.com/jayjiahua/phantomjs-bin-pip/issues/1 | ||
| import warnings | ||
| warnings.warn('Phantomjs executable %s is not executable. Updating permissions' % (phantomjs_exe), |
There was a problem hiding this comment.
I should have mentioned this before, but one should essentially never use a dynamic warning string. The old code did, and the old code was wrong.
A dynamic warning string is both hard to filter using the provided mechanisms (e.g., string matching based on an environment variable), and also can be a leak.
The warnings module strives to only emit a warning one time for any given location. It does this by keeping track of the stack location and the string that was emitted. If it sees the same (location, string) pair again, it doesn't emit the warning. Otherwise, it makes a new entry and emits the warning.
The old code didn't have a problem here because it could only ever be emitted once anyway (at import time). This code could have an issue because it can be run multiple times from different threads simultaneously.
Now, the dynamic string is typically going to format to the same value every time. Will the warnings cache account for that, or will it be based on object identity? No idea (actually, in CPython, warnings is implemented in C, and the easiest way to do such a cache would be to use a standard dictionary, so it would be value based, but lets pretend we don't know that).
How about using the logger here? Logging wasn't set up at module import time, but it should be by the time this executes. (The old code should have used a custom Warning object subclass.)
There was a problem hiding this comment.
TIL.
Works for me. Thanks.
|
The best part of this is I get to mark another project as running under compattest.... |
The current release of phantomjs_bin has binaries that aren't executable. jayjiahua/phantomjs-bin-pip#1
At import time we check the binary we are going to use is user executable and attempt to fix it if it is not. This feels a bit dangerous, perhaps there is a better way to go about this?
Fixes Issue #77