On a Mac, check if xcode installed before requiring watchdog by jrhone · Pull Request #91 · streamlit/streamlit

jrhone · 2019-09-11T12:02:29Z

Issue #66

Modify setup.py so we check whether xcode tools are available before making watchdog a dependency (only if the current system is a Mac, of course)
In LocalSourcesWatcher.py, when we fall back to the polling solution, print a message telling the user to install the watchdog module
Add config option to turn that message off: global.disableWatchdogWarning

tvst · 2019-09-12T19:40:45Z

lib/streamlit/config.py

+    'global.disableWatchdogWarning',
+    description='''
+        If False, will show a warning if the watchdog module is not available
+        on the user's system and we fall back to the polling file watcher.


To avoid double negatives, change to:

By default, Streamlit checks if the Python watchdog module is available and, if not, prints a warning asking for you to install it. The watchdog module is not required, but highly recommended. It improves Streamlit's ability to detect changes to files in your filesystem.

If you'd like to turn off this warning, set this to True.

lib/streamlit/watcher/LocalSourcesWatcher.py

tvst · 2019-09-12T19:42:54Z

lib/tests/streamlit/config_test.py

            u'client.caching',
            u'client.displayEnabled',
            u'global.developmentMode',
+            u'global.disableWatchdogWarning',


I guess this is not easy to test, huh? Any ideas what we could do here?

Hmm maybe import LocalSourcesWatcher and see if the output is captured by capsys?

https://docs.pytest.org/en/latest/capture.html#accessing-captured-output-from-a-test-function

That would work, but you'd have to also:

Remove LocalSourcesWatcher from sys.modules before running the test, so the module gets imported at test time (in case it had previously been imported by another test)

Mock a bunch of things, like the function that tells you whether you're on a Mac

monchier · 2019-09-12T17:51:48Z

lib/setup.py

+# Check whether xcode tools are available before making watchdog a
+# dependency (only if the current system is a Mac).
+if (platform.system() == 'Darwin' and
+        subprocess.call('xcode-select --version', shell=True) != 0):


can we use shell=False
https://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess

shell=True is usually deemed unsecure.

shell=True is insecure if you're executing user generated input, as far as I understand. It also wasn't working without it, unless I was doing something incorrectly.

Warning Executing shell commands that incorporate unsanitized input from an untrusted source makes a program vulnerable to shell injection, a serious security flaw which can result in arbitrary command execution. For this reason, the use of shell=True is strongly discouraged in cases where the command string is constructed from external input:

https://docs.python.org/2/library/subprocess.html#frequently-used-arguments

I think you can use shell=False by changing the first argument to ('xcode-select', '--version')

monchier · 2019-09-12T17:53:20Z

lib/streamlit/config.py

+    'global.disableWatchdogWarning',
+    description='''
+        If False, will show a warning if the watchdog module is not available
+        on the user's system and we fall back to the polling file watcher.


Does this option causes to fall back on the 'polling'? I think it does not, maybe we could reword this?

reworded with Thiago's suggestion

monchier · 2019-09-12T17:55:02Z

lib/streamlit/watcher/LocalSourcesWatcher.py


-from streamlit.logger import get_logger
-LOGGER = get_logger(__name__)
+  $ xcode-select --install


Will this be shown on Linux as well if the option is true and the watchdog is not installed? Is the watchdog always installed on Linux?

Watchdog is in the requirements and will be installed when the user installs Streamlit. I suppose they could uninstall watchdog afterwards and this message would be displayed.

tvst · 2019-09-13T21:37:03Z

lib/setup.py

+# Check whether xcode tools are available before making watchdog a
+# dependency (only if the current system is a Mac).
+if (platform.system() == 'Darwin' and
+        subprocess.call('xcode-select --version', shell=True) != 0):


I think you can use shell=False by changing the first argument to ('xcode-select', '--version')

lib/streamlit/watcher/LocalSourcesWatcher.py

tvst · 2019-09-13T21:41:05Z

lib/tests/streamlit/config_test.py

            u'client.caching',
            u'client.displayEnabled',
            u'global.developmentMode',
+            u'global.disableWatchdogWarning',


That would work, but you'd have to also:

Remove LocalSourcesWatcher from sys.modules before running the test, so the module gets imported at test time (in case it had previously been imported by another test)

Mock a bunch of things, like the function that tells you whether you're on a Mac

tvst · 2019-09-13T21:41:36Z

lib/setup.py


+# Check whether xcode tools are available before making watchdog a
+# dependency (only if the current system is a Mac).
+if (platform.system() == 'Darwin' and


Maybe we should pull this into util.py, since it seems we'll need to do this check in a few different places.

different log message for darwin vs not

* develop: (54 commits) Removing uber demo from streamlit repo (streamlit#159) Release 0.46.0 (streamlit#170) Magic fixes (streamlit#138) [docs] Add analytics; redirect /secret/docs; fix compilation problem (streamlit#149) Fix bug for startup under windows (streamlit#144) Responsive layout (streamlit#104) Add basic PR template Better method signatures (streamlit#88) Publish docs to both /docs and /secret/docs, until we deprecate /secret/docs in January. (streamlit#141) Rename/report2app (streamlit#126) Test running streamlit commands "bare" (streamlit#133) Updates to LP and sidebar. (streamlit#134) tests for z-index of date input popover in sidebar (streamlit#131) cypress test for escaping html in markdown (streamlit#125) On a Mac, check if xcode installed before requiring watchdog (streamlit#91) [Docs] Fix st.slider API in tutorial (streamlit#98) Sidebar exceptions (streamlit#107) Fixing unbound local variable (streamlit#110) Support hashing dataframes with unhashable objects. Gracefully f… (streamlit#118) Fix hashing if the object has a name but the name is not a string. (streamlit#117) ...

remove watchdog from requirements if on a mac xcode tools not installed

1e96907

jrhone requested review from arraydude, kantuni, monchier, tconkling and tvst and removed request for arraydude, kantuni, monchier, tconkling and tvst September 11, 2019 12:02

jrhone changed the title ~~On a Mac, check if xcode installed before requiring watchdog~~ WIP: On a Mac, check if xcode installed before requiring watchdog Sep 11, 2019

fix missing config in test

da69e36

jrhone changed the title ~~WIP: On a Mac, check if xcode installed before requiring watchdog~~ On a Mac, check if xcode installed before requiring watchdog Sep 11, 2019

jrhone requested review from arraydude, kantuni, monchier, tconkling and tvst September 11, 2019 12:28

tvst approved these changes Sep 12, 2019

View reviewed changes

monchier reviewed Sep 12, 2019

View reviewed changes

tvst approved these changes Sep 13, 2019

View reviewed changes

jrhone added 4 commits September 16, 2019 13:37

update disable watchdog config description

5f3aa2f

set shell=false when invoking subprocess to check xcode-select version

51ae7ca

move platform check into util.py

2cd92e2

different log message for darwin vs not

Merge branch 'develop' into xcode_watchdog

356acfb

jrhone merged commit f38a7c5 into streamlit:develop Sep 17, 2019

Conversation

jrhone commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhone Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhone Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jrhone commented Sep 11, 2019 •

edited

Loading

jrhone Sep 13, 2019 •

edited

Loading

jrhone Sep 13, 2019 •

edited

Loading