You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@toree.apache.org by GitBox <gi...@apache.org> on 2019/12/23 07:02:48 UTC
[GitHub] [incubator-toree] lresende opened a new pull request #181:
[TOREE-511] Rework make jupyter build tasks
lresende opened a new pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181
- Create a proper docker image for dev
- Combine dev/jupyter build tasks into jupyter
- Use JupyterLab as default UI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r362651592
##########
File path: .gitignore
##########
@@ -51,7 +51,7 @@ apache-rat-*.jar
**.swp
**/*ivy.xml
.vagrant
-.example-image
+.toree-dev-image
Review comment:
Will likely need to add `.binder-image` once the `touch` is added (per other comment).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r362650343
##########
File path: Makefile
##########
@@ -81,19 +83,15 @@ clean: VM_WORKDIR=/src/toree-kernel
clean: clean-dist
$(call RUN,$(ENV_OPTS) sbt clean)
rm -r `find . -name target -type d`
+ -rm -r `find . -name .ipynb_checkpoints -type d`
-.example-image: EXTRA_CMD?=printf "deb http://cran.rstudio.com/bin/linux/debian jessie-cran3/" >> /etc/apt/sources.list; apt-key adv --keyserver keys.gnupg.net --recv-key 381BA480; apt-get update; pip install jupyter_declarativewidgets==0.4.4; jupyter declarativewidgets install --user; jupyter declarativewidgets activate; pip install jupyter_dashboards; jupyter dashboards install --user; jupyter dashboards activate; apt-get update; apt-get install --yes curl; curl --silent --location https://deb.nodesource.com/setup_0.12 | sudo bash -; apt-get install --yes nodejs r-base r-base-dev; npm install -g bower;
-.example-image:
- @-docker rm -f examples_image
- @docker run -t --user root --name examples_image \
- $(IMAGE) bash -c '$(EXTRA_CMD)'
- @docker commit examples_image $(EXAMPLE_IMAGE)
- @-docker rm -f examples_image
- touch $@
+clean-images:
+ -rm -r .toree-dev-image
+ -rm -r .binder-image
Review comment:
Should this target actually remove the images as well? If so, `clean-images` would depend on `clean-toree-dev-image` and `clean-binder-image` - with those added to `.PHONY` and ignore the same comment regarding `clean-images` since it now depends on other targets.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r363778165
##########
File path: Makefile
##########
@@ -83,21 +87,19 @@ clean: clean-dist
rm -r `find . -name target -type d`
-rm -r `find . -name .ipynb_checkpoints -type d`
-.example-image: EXTRA_CMD?=printf "deb http://cran.rstudio.com/bin/linux/debian jessie-cran3/" >> /etc/apt/sources.list; apt-key adv --keyserver keys.gnupg.net --recv-key 381BA480; apt-get update; pip install jupyter_declarativewidgets==0.4.4; jupyter declarativewidgets install --user; jupyter declarativewidgets activate; pip install jupyter_dashboards; jupyter dashboards install --user; jupyter dashboards activate; apt-get update; apt-get install --yes curl; curl --silent --location https://deb.nodesource.com/setup_0.12 | sudo bash -; apt-get install --yes nodejs r-base r-base-dev; npm install -g bower;
-.example-image:
- @-docker rm -f examples_image
- @docker run -t --user root --name examples_image \
- $(IMAGE) bash -c '$(EXTRA_CMD)'
- @docker commit examples_image $(EXAMPLE_IMAGE)
- @-docker rm -f examples_image
- touch $@
+.clean-toree-dev-image:
+ if [ -f .toree-dev-image ]; then rm -r .toree-dev-image; fi
Review comment:
Since `.toree-dev-image` is just a file and not a directory, this line can be replaced with...
`@rm -f .toree-dev-image`
`-f` removes the need to test of existence.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r362651030
##########
File path: Makefile
##########
@@ -81,19 +83,15 @@ clean: VM_WORKDIR=/src/toree-kernel
clean: clean-dist
$(call RUN,$(ENV_OPTS) sbt clean)
rm -r `find . -name target -type d`
+ -rm -r `find . -name .ipynb_checkpoints -type d`
-.example-image: EXTRA_CMD?=printf "deb http://cran.rstudio.com/bin/linux/debian jessie-cran3/" >> /etc/apt/sources.list; apt-key adv --keyserver keys.gnupg.net --recv-key 381BA480; apt-get update; pip install jupyter_declarativewidgets==0.4.4; jupyter declarativewidgets install --user; jupyter declarativewidgets activate; pip install jupyter_dashboards; jupyter dashboards install --user; jupyter dashboards activate; apt-get update; apt-get install --yes curl; curl --silent --location https://deb.nodesource.com/setup_0.12 | sudo bash -; apt-get install --yes nodejs r-base r-base-dev; npm install -g bower;
-.example-image:
- @-docker rm -f examples_image
- @docker run -t --user root --name examples_image \
- $(IMAGE) bash -c '$(EXTRA_CMD)'
- @docker commit examples_image $(EXAMPLE_IMAGE)
- @-docker rm -f examples_image
- touch $@
+clean-images:
+ -rm -r .toree-dev-image
+ -rm -r .binder-image
-.system-test-image:
- @-docker rm -f $(SYSTEM_TEST_IMAGE)
- @docker build -t $(SYSTEM_TEST_IMAGE) -f Dockerfile.system-test .
+.toree-dev-image:
+ @-docker rm -f $(TOREE_DEV_IMAGE)
+ @docker build -t $(TOREE_DEV_IMAGE) -f Dockerfile.toree-dev .
touch $@
.binder-image:
Review comment:
I think this target needs a `touch $@` (and image removal - similar to `.toree-dev-image`).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r363776548
##########
File path: Makefile
##########
@@ -15,7 +15,10 @@
# limitations under the License
#
-.PHONY: help clean clean-dist build dev test system-test test-travis release pip-release bin-release dev-binder .binder-image audit audit-licenses
+.PHONY: help clean clean-dist .clean-binder-image .clean-toree-dev-image \
+ build dev test system-test test-travis release pip-release bin-release \
+ dev-binder .binder-image .toree-dev-image \
Review comment:
`.binder-image` and `.toree-dev-image` should not be listed here as they correspond to files that _are_ the target.
Also, do we need the '.' prefix on `.clean-binder-image` and `.clean-toree-dev-image`? I think the dotted notation is purely because we don't want the files visible by default. Because these are PHONY targets, I think removing the dotted prefix is appropriate.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] lresende commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
lresende commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r363805182
##########
File path: Makefile
##########
@@ -15,7 +15,10 @@
# limitations under the License
#
-.PHONY: help clean clean-dist build dev test system-test test-travis release pip-release bin-release dev-binder .binder-image audit audit-licenses
+.PHONY: help clean clean-dist .clean-binder-image .clean-toree-dev-image \
+ build dev test system-test test-travis release pip-release bin-release \
+ dev-binder .binder-image .toree-dev-image \
Review comment:
regarding the dot, I was just following the same pattern as others
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r363777123
##########
File path: Makefile
##########
@@ -83,21 +87,19 @@ clean: clean-dist
rm -r `find . -name target -type d`
-rm -r `find . -name .ipynb_checkpoints -type d`
-.example-image: EXTRA_CMD?=printf "deb http://cran.rstudio.com/bin/linux/debian jessie-cran3/" >> /etc/apt/sources.list; apt-key adv --keyserver keys.gnupg.net --recv-key 381BA480; apt-get update; pip install jupyter_declarativewidgets==0.4.4; jupyter declarativewidgets install --user; jupyter declarativewidgets activate; pip install jupyter_dashboards; jupyter dashboards install --user; jupyter dashboards activate; apt-get update; apt-get install --yes curl; curl --silent --location https://deb.nodesource.com/setup_0.12 | sudo bash -; apt-get install --yes nodejs r-base r-base-dev; npm install -g bower;
-.example-image:
- @-docker rm -f examples_image
- @docker run -t --user root --name examples_image \
- $(IMAGE) bash -c '$(EXTRA_CMD)'
- @docker commit examples_image $(EXAMPLE_IMAGE)
- @-docker rm -f examples_image
- touch $@
+.clean-toree-dev-image:
+ if [ -f .toree-dev-image ]; then rm -r .toree-dev-image; fi
+ @-docker rmi -f $(TOREE_DEV_IMAGE)
-.system-test-image:
- @-docker rm -f $(SYSTEM_TEST_IMAGE)
- @docker build -t $(SYSTEM_TEST_IMAGE) -f Dockerfile.system-test .
+.toree-dev-image:
+ @docker build -t $(TOREE_DEV_IMAGE) -f Dockerfile.toree-dev .
touch $@
-.binder-image:
+.clean-binder-image:
+ if [ -f .binder-image ]; then rm -r .binder-image; fi
+ @-docker rmi -f $(BINDER_IMAGE)
+
+.binder-image: .clean-binder-image
@docker build --rm -t $(BINDER_IMAGE) .
Review comment:
This target needs `touch $@` to create the `.binder-image` file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r362649415
##########
File path: Makefile
##########
@@ -63,12 +63,14 @@ help:
@echo ' '
@echo ' audit - run audit tools against the source code'
@echo ' clean - clean build files'
- @echo ' dev - starts ipython'
+ @echo ' clean-images - clean docker images used in examples and '
Review comment:
The command's description is incomplete.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] lresende merged pull request #181: [TOREE-511]
Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
lresende merged pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r363778369
##########
File path: Makefile
##########
@@ -83,21 +87,19 @@ clean: clean-dist
rm -r `find . -name target -type d`
-rm -r `find . -name .ipynb_checkpoints -type d`
-.example-image: EXTRA_CMD?=printf "deb http://cran.rstudio.com/bin/linux/debian jessie-cran3/" >> /etc/apt/sources.list; apt-key adv --keyserver keys.gnupg.net --recv-key 381BA480; apt-get update; pip install jupyter_declarativewidgets==0.4.4; jupyter declarativewidgets install --user; jupyter declarativewidgets activate; pip install jupyter_dashboards; jupyter dashboards install --user; jupyter dashboards activate; apt-get update; apt-get install --yes curl; curl --silent --location https://deb.nodesource.com/setup_0.12 | sudo bash -; apt-get install --yes nodejs r-base r-base-dev; npm install -g bower;
-.example-image:
- @-docker rm -f examples_image
- @docker run -t --user root --name examples_image \
- $(IMAGE) bash -c '$(EXTRA_CMD)'
- @docker commit examples_image $(EXAMPLE_IMAGE)
- @-docker rm -f examples_image
- touch $@
+.clean-toree-dev-image:
+ if [ -f .toree-dev-image ]; then rm -r .toree-dev-image; fi
+ @-docker rmi -f $(TOREE_DEV_IMAGE)
-.system-test-image:
- @-docker rm -f $(SYSTEM_TEST_IMAGE)
- @docker build -t $(SYSTEM_TEST_IMAGE) -f Dockerfile.system-test .
+.toree-dev-image:
+ @docker build -t $(TOREE_DEV_IMAGE) -f Dockerfile.toree-dev .
touch $@
-.binder-image:
+.clean-binder-image:
+ if [ -f .binder-image ]; then rm -r .binder-image; fi
Review comment:
Same comment as `.toree-dev-image` removal above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r362649694
##########
File path: Makefile
##########
@@ -81,19 +83,15 @@ clean: VM_WORKDIR=/src/toree-kernel
clean: clean-dist
$(call RUN,$(ENV_OPTS) sbt clean)
rm -r `find . -name target -type d`
+ -rm -r `find . -name .ipynb_checkpoints -type d`
-.example-image: EXTRA_CMD?=printf "deb http://cran.rstudio.com/bin/linux/debian jessie-cran3/" >> /etc/apt/sources.list; apt-key adv --keyserver keys.gnupg.net --recv-key 381BA480; apt-get update; pip install jupyter_declarativewidgets==0.4.4; jupyter declarativewidgets install --user; jupyter declarativewidgets activate; pip install jupyter_dashboards; jupyter dashboards install --user; jupyter dashboards activate; apt-get update; apt-get install --yes curl; curl --silent --location https://deb.nodesource.com/setup_0.12 | sudo bash -; apt-get install --yes nodejs r-base r-base-dev; npm install -g bower;
-.example-image:
- @-docker rm -f examples_image
- @docker run -t --user root --name examples_image \
- $(IMAGE) bash -c '$(EXTRA_CMD)'
- @docker commit examples_image $(EXAMPLE_IMAGE)
- @-docker rm -f examples_image
- touch $@
+clean-images:
Review comment:
Since this target doesn't include a dependency, I think it should be added to the `.PHONY` target.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-toree] kevin-bates commented on a change in pull request
#181: [TOREE-511] Rework make jupyter build tasks
Posted by GitBox <gi...@apache.org>.
kevin-bates commented on a change in pull request #181: [TOREE-511] Rework make jupyter build tasks
URL: https://github.com/apache/incubator-toree/pull/181#discussion_r362650724
##########
File path: Makefile
##########
@@ -81,19 +83,15 @@ clean: VM_WORKDIR=/src/toree-kernel
clean: clean-dist
$(call RUN,$(ENV_OPTS) sbt clean)
rm -r `find . -name target -type d`
+ -rm -r `find . -name .ipynb_checkpoints -type d`
-.example-image: EXTRA_CMD?=printf "deb http://cran.rstudio.com/bin/linux/debian jessie-cran3/" >> /etc/apt/sources.list; apt-key adv --keyserver keys.gnupg.net --recv-key 381BA480; apt-get update; pip install jupyter_declarativewidgets==0.4.4; jupyter declarativewidgets install --user; jupyter declarativewidgets activate; pip install jupyter_dashboards; jupyter dashboards install --user; jupyter dashboards activate; apt-get update; apt-get install --yes curl; curl --silent --location https://deb.nodesource.com/setup_0.12 | sudo bash -; apt-get install --yes nodejs r-base r-base-dev; npm install -g bower;
-.example-image:
- @-docker rm -f examples_image
- @docker run -t --user root --name examples_image \
- $(IMAGE) bash -c '$(EXTRA_CMD)'
- @docker commit examples_image $(EXAMPLE_IMAGE)
- @-docker rm -f examples_image
- touch $@
+clean-images:
+ -rm -r .toree-dev-image
+ -rm -r .binder-image
-.system-test-image:
- @-docker rm -f $(SYSTEM_TEST_IMAGE)
- @docker build -t $(SYSTEM_TEST_IMAGE) -f Dockerfile.system-test .
+.toree-dev-image:
+ @-docker rm -f $(TOREE_DEV_IMAGE)
Review comment:
If `clean-images` removes the images, then this target would depend on `clean-toree-dev-image` and that target would include the `docker rm` command.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services