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

Install IJulia kernel into proper directory #622

Merged
merged 9 commits into from Mar 21, 2019

Conversation

@davidanthoff
Copy link
Contributor

davidanthoff commented Mar 12, 2019

Fixes #620.

I haven't tested this PR yet, this is more a blind edit. I'll try to run this later today, just wanted to signal that there is a fix.

@davidanthoff davidanthoff referenced this pull request Mar 12, 2019
1 of 3 tasks complete
@davidanthoff davidanthoff changed the title WIP Install IJulia kernel into proper directory Install IJulia kernel into proper directory Mar 12, 2019
@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 13, 2019

Now tested the Project.toml file case, seems to work. When I try to test the REQUIRE case, I get an error that there is not enough space to clone METADATA.jl, which to me sounds like some docker issue that is probably unrelated...

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Mar 14, 2019

Did rerunning the tests fix the problem? This looks like it should work to me!

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 14, 2019

This was a bit of a meandering path, but it uncovered a bug in the previous implementation and should work now. Long story short, the whole passing-a-dict-with-env-vars to installkernel did actually not work at all: all it does is set these env vars for the kernel process, not for the process that installs a kernel. That also explains why #621 worked :) So now I went back to the implementation that @rabernat used there, which actually is the proper way to do this.

In any case, I think this is now ready to be merged.

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 19, 2019

@betatim could we merge this? It fixes a bug in the existing master version.

@davidanthoff davidanthoff referenced this pull request Mar 20, 2019
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 21, 2019

I was on vacation last week so still trying to catch up with things.

betatim and others added 2 commits Mar 21, 2019
Co-Authored-By: davidanthoff <anthoff@berkeley.edu>
@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 21, 2019

Thanks @betatim! I accepted your suggestion, and also made that change in the other places.

So I think at this point this PR is really purely a bug-fix: the way I had previously set JUPYTER_DATA_DIR was just an error and didn't do anything, with this PR it now gets set in such a way that the IJulia build step picks it up. The location we are setting, though, is still the same that we were trying to set before.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 21, 2019

Perfect! Waiting for travis to be happy and then it'll get merged.

Thanks for the patience :) (I might have been overly grumpy for unrelated reasons this morning)

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 21, 2019

Haha, I did not perceive it as grumpy at all, no worries!

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Mar 21, 2019

Haha I would also second @davidanthoff's previous observations that the reviewers in this repo are some of the friendliest, quickest, and most thorough reviewers I've worked with, so you're afforded some grumpiness every once in a while given your large credit of charm and good will! 😋

(Also, really, just everyone in Jupyter in general is lovely, which I very much appreciate!!)

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 21, 2019

Travis is happy. Merging!

@davidanthoff do you want to deploy these changes to mybinder.org? We have instructions on making a new deployment that should let you do it. Ping me when you need a merge for the deploy PR.

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 21, 2019

Great! I'd like to finish #612 first, and then deploy? I'll ping you once that PR is ready.

@betatim betatim merged commit e921993 into jupyter:master Mar 21, 2019
5 checks passed
5 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch Coverage not affected when comparing 7644d58...ae86baa
Details
codecov/project 90.75% remains the same compared to 7644d58
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidanthoff davidanthoff deleted the davidanthoff:ijulia-kernel-install-location branch Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.