Fix mac install

[blag]

Fixes some things on Mac by creating a separate virtualenv in virtualenv-osx, so you can run stuff in a (Linux) docker container and still do stuff in OS X.

I will try to include a fix for #3934 by implementing #3785 before this PR is ready. Done.

Also (for now) includes the additional logging from #4141.

Reviewers: please check the changes to the relative virtualenv path in tools/launchdev.sh, as I'm not 100% sure where that command is supposed to be run from. This PR changes the virtualenv to the virtualenv directory at the root of the repo, because that's what makes the most sense to me.

Also, my implementation in tail.py works, but I'm not sure if it's the most optimal threading model.
Fixed. It now uses eventlet greenthreads and a greenpool.
Handled in #4191.

[lakshmi-kannan]

While I like the PR, this would make people develop on mac and not use docker dev env. I guess choice is good. The other thing is we had issues with eventlet monkey patching and OS X filesystem (case preserving but insensitive - cat CAT.txt and cat cat.txt are equivalent but you could create both the files IIRC). So OS X is a gray area.

[lakshmi-kannan]

Basically, see if all unit tests pass and lanuchdev.sh works ok. That's a good starting point for sanity on OS X.

[blag]

@lakshmi-kannan You raise good points, but I think devs will switch to the docker dev env since that seems to consistently work better for coding.

However, this is still useful for non-code dev work, like documentation fixes, on OS X. From #3934:

LindsayHill:

As an aside, this would make my life a little easier when doing st2docs work on Mac.

tonybaloney:

LindsayHill you read my mind :-) it would make my life easier to be able to run the unit tests on my Mac and not have to sync the repo with a server for minor changes.

[enykeev]

It seems to me that it would be fair to add a note during virtualenv-osx creation to let user know that OSX is not officially supported at that point and the results he'll get by running st2 in OSX may not represent of what he will get in linux (false positives\negatives in tests that will later are going to came during CI, problems related to default case-insensitive behavior of HFS+, outdated native dependencies and so on).

I agree that user should be able to do some development on osx. I also believe that the problems with osx compatibility should be uncovered and eventually resolved even though our main focus continues to be in linux env.

[Kami]

Because of that, I assume this failed is based on logshipper?

[Kami]

Also, how hard would it be / would it make sense to contribute those changes upstream?

[blag]

Yes, I copied and pasted that file out of logshipper. Both StackStorm and logshipper are licensed under the Apache 2.0 license though, so there shouldn't be a legal issue.

Upstream logshipper hasn't been updated in 3-4 years. The author hasn't responded to any issues or pull requests in that long, and the last commit was on Mar 23, 2015. I would be happy to upstream this if I thought it had a chance to get merged, but I don't think the original author is interested in the project anymore.

[Kami]

If possible, I would prefer to split out / re-organize the code so it's easy to see which code is original and which is our modifications.

I know that might not be possible though, or at least not easily.

[blag]

@Kami or @enykeev I'll merge this in once one of you reviews it.

[lakshmi-kannan]

Why are we installing pika when we have kombu?

[blag]

The tools/direct_queue_publisher.py imports pika, and that was messing up the pylint check.

[lakshmi-kannan]

We don't even use that tool. Maybe switch that tool to use kombu?

[bigmstone]

I don't use it, but how is this a new problem? Should have been caught previously. Kombu installs pika as a dep. Should still work if venv is set up.

[blag]

If we don't use that tool we should just remove it entirely. I'm also not entirely sure why any of my changes caused pylint to check (and then complain) about this file when it wasn't doing it before, but ¯\_(ツ)_/¯.

I added this quick hack because without it the Travis tests were failing due to pylint complaining that pika wasn't installed.

[lakshmi-kannan]

On a brand new OS X, make requirements fails

lakshmi@Lakshmis-MacBook-Pro ~/Work/src/storm/st2 (fix-mac-install)$ make requirements                        [ruby-2.3.3p222]

==================== virtualenv ====================

# Note: We pass --no-download flag to make sure version of pip which we install (9.0.1) is used
# instead of latest version being downloaded from PyPi
# For OS X, we have to bootstrap install pip because otherwise it runs into
# SSLv1 errors when connecting to https://pypi.python.org/simple/ to
# download anything
/bin/bash: virtualenv: command not found

[lakshmi-kannan]

I'd install pip first and then virtualenv because stock OS X doesn't come with neither. We can also assume people have pip and virtualenv installed in which case you should just check if they exist and if not ask user to install those.

[blag]

The situation here is a little more complicated. This code actually does assume that the user has some version of virtualenv installed, and the most common way to install virtualenv is with pip, so if users have virtualenv, they probably also have a version of pip.

The issue with pip on OS X is that you might run into this issue, where you need a recent version of pip to install or upgrade anything including pip itself. So if you have a version of pip that is out of date, you cannot use it to do anything.

The problem is that you need to install a recent version of pip into the virtualenv, but you cannot use pip to do it. The method I use to install/upgrade pip here is pretty bulletproof (although it its a little scary from a security perspective) and is an official way to install pip.

[lakshmi-kannan]

Can you set ST2_REPO to ${PWD} if ST2_REPO is not set? Otherwise, I end up with

tools/launchdev.sh: line 223: /virtualenv-osx/bin/activate: No such file or directory

[blag]

Good catch!

Done (I think): https://github.com/StackStorm/st2/pull/4145/files#diff-b4e17371e9e3b263027f6a393d3bd772R9

[lakshmi-kannan]

I think this is pretty simple and yet clever :)

[blag]

Thanks!

[lakshmi-kannan]

We don't even use that tool. Maybe switch that tool to use kombu?

[Kami]

I tried checkout out this branch, recreating the virtualen environment to make sure requirements are up to date and starting the sensor, but I get the following error on Linux - https://gist.github.com/Kami/8972f8da8dfedb9eb1269f1612cca78f

I would imagine it's probably related to eventlet select module monkey patch. I thought it's maybe related to code change in https://github.com/StackStorm/st2/pull/4118/files, but even if I remove those changes I get the same error locally.

[Kami]

I could get it to work if I removed eventlet monkey patch (but only partially since it immediately crashed after the file was modified).

To get this to work, we have a couple of options:

1. Modify inotify implementation so it's eventlet compatible (probably not that easy and a lot of work)
2. Add the same work around we have here for select.poll also for select.epoll - https://github.com/StackStorm/st2/pull/4118/files#diff-edec9adda8cdf5199d2b026e5c4c3fceR76

I'm personally not a big fan of 2) approach, we already have more hacks / workarounds than we would like to have.

So with the pyinotify working out of the box, I wonder if those changes are still good idea if we can't get it to work with 1).

[Kami]

Here is a list of two different changes with which I could get the sensor to start:

1. Remove eventlet monkey patch (just to test it, not feasible as a solution)

diff --git a/st2reactor/st2reactor/container/sensor_wrapper.py b/st2reactor/st2reactor/container/sensor_wrapper.py
index dfcc7ab2e..36bad6acf 100644
--- a/st2reactor/st2reactor/container/sensor_wrapper.py
+++ b/st2reactor/st2reactor/container/sensor_wrapper.py
@@ -46,8 +46,8 @@ __all__ = [
     'SensorService'
 ]
 
-monkey_patch()
-use_select_poll_workaround(nose_only=False)
+#monkey_patch()
+#use_select_poll_workaround(nose_only=False)
 
 
 class SensorService(object):

2. Add the same eventlet select workaround for select.epoll as we have for select.poll

diff --git a/st2common/st2common/util/monkey_patch.py b/st2common/st2common/util/monkey_patch.py
index b83d74a4f..38435868d 100644
--- a/st2common/st2common/util/monkey_patch.py
+++ b/st2common/st2common/util/monkey_patch.py
@@ -77,7 +77,9 @@ def use_select_poll_workaround(nose_only=True):
     if not nose_only or (nose_only and'nose' in sys.modules.keys()):
         # Add back blocking poll() to eventlet monkeypatched select
         original_poll = eventlet.patcher.original('select').poll
+        original_epoll = eventlet.patcher.original('select').epoll
         select.poll = original_poll
+        select.epoll = original_epoll
 
         sys.modules['select'] = select
         subprocess.select = select

As mentioned above, I'm not a huge fan of another workaround, especially because pyinotify code worked out of the box.

My main question with those code changes is what do we gain over the previous approach if we still need to maintain a fork (this time the code is inside this repo and not inside another one, not any better, imo) and we need additional hacks / workarounds.

[Kami]

Also, would it be possible to split file watch changes in a separate branch / PR?

This way we could review OS X changes independently and hopefully get them merged faster.

[Kami]

As far as mac changes go - can we also add some tests for that?

Travis CI supports osx OS so we should add some checks tests / checks, if possible.

[blag]

Travis CI supports Python and it supports Mac, but it doesn't natively support Python on Mac. There are some ways to do it, but we don't support running ST2 itself or tests on OS X, so I don't know what that would buy us.

[bigmstone]

As discussed in slack - I'm generally -1 on this change primarily because of two things. The additions to the Makefile and the additional virtualenv. It's our build system and we should hope to keep it as clean as possible and with as minimal corner cases covered since we have the luxury of being opinionated about it. We have to maintain every line, after all. I'd rather the effort going into making devbox (docker backed) or vagrant as clean as possible and that be the "one-true" way to do things (developers will always do what they want, but that shouldn't make it into master.)

Don't consider this comment a blocker, just me voicing my concern.

I'm generally +1 to the linux tail change.