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

Conda package #113

Merged
merged 25 commits into from
Aug 9, 2018
Merged

Conda package #113

merged 25 commits into from
Aug 9, 2018

Conversation

jonmmease
Copy link
Contributor

Adds conda recipe and updates CI jobs to build conda packages for OSX, Linux, and Windows.

Supersedes #108 due to CI permission issues.

Add Python logic to convert npm version string to setuptools
standard version string (1.0.1rc1 in this case)
Environment variables aren't expanded here
Environment variables aren't expanded here
e.g. conda-linux-64.zip -> linux-64/*.tar.bz2
This causes the bash process to become the orca process, which means
that signals will be processed by the orca executable directly.
See http://veithen.github.io/2014/11/16/sigterm-propagation.html

This fixes a problem where it was not possible to kill an orca server
from Python if it was launched through the wrapper script using
subprocess.Popen
@jonmmease jonmmease mentioned this pull request Aug 9, 2018
@jonmmease
Copy link
Contributor Author

@etpinard green here too! Was there anything else you wanted to look over for this one?

@etpinard
Copy link
Contributor

etpinard commented Aug 9, 2018

@jonmmease have you tested out d6f9c1e on a linux machine? If not, I can do it no problem

@jonmmease
Copy link
Contributor Author

No I didn't, that's good point. I should also check whether there's an analogous issue on Windows.

I'll test both using my same Python script and let you know what I find.

@jonmmease
Copy link
Contributor Author

@etpinard I tested in on Ubuntu and it works just like on OS X. I tested it on Windows 10 and I had to use some different logic from Python to kill the process, but I was able to so there's nothing that needs to change on the Windows side.

Anything else come to mind?

@etpinard
Copy link
Contributor

etpinard commented Aug 9, 2018

I tested in on Ubuntu and it works just like on OS X.

Amazing!

ything else come to mind?

As this PR updates our README with new installation instructions, I'll like to wait until the conda channel thing is sorted out (cc @chriddyp ) before merging this thing and quickly release v1.1.0 right after.

@jonmmease
Copy link
Contributor Author

I could back-out the README change and put that in a new PR for the 1.1.0 release. If we bump the version number in that branch we could go ahead and publish the conda packages, and test them from the anaconda channel, before merging it into master. We could also do the changelog on that branch (I didn't put anything in there for the --graph-only option yet).

@etpinard
Copy link
Contributor

etpinard commented Aug 9, 2018

I could back-out the README change and put that in a new PR for the 1.1.0 release.

I'd say do whatever is easier for you. If you think having these commits on master now will help your devflow, then revert back the README changes and merge this thing 💃

If we bump the version number in that branch we could go ahead and publish the conda packages, and test them from the anaconda channel, before merging it into master. We could also do the changelog on that branch (I didn't put anything in there for the --graph-only option yet).

I usually bump the package version on my local copy of master and push it (along with the git tags) straight to origin master. I update the changelog in a single commit in one go (no need to do this during every PR) before the version commit and copy its content to https://github.com/plotly/orca/releases. The complete checklist is here and we essentially use the same one for plotly.js

That said, I'm open to suggestions if you think we could do better.

@jonmmease
Copy link
Contributor Author

Thanks for explaining, that makes sense. I'll go ahead and revert the README change and then merge this, that way I can start building the conda packages I need for plotly.py dev off of master.

Do you want to just pull the README off of this branch when you do the release?

@etpinard
Copy link
Contributor

etpinard commented Aug 9, 2018

Do you want to just pull the README off of this branch when you do the release?

Yep. I'll do that. Thanks!


Once you're happy with the state of master, I'll release v1.1.0.

We'll bring these back in during the release process.
@etpinard
Copy link
Contributor

etpinard commented Aug 9, 2018

Brilliant 💃

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.

2 participants