Add Dockerfile and instructions#13
Add Dockerfile and instructions#13zslayton merged 6 commits intoamazon-ion:masterfrom camerondurham:master
Conversation
* Added Dockerfile to build minimal, multi-stage image * Updated README with build instructions Recording of local build/test: https://asciinema.org/a/Ql7Xc5fgxxvkFrjjcuMbR0npY Currently builds image size 85.8MB from local build on macOS ``` docker images REPOSITORY TAG IMAGE ID CREATED SIZE ion-cli 0.1.1 e81a9d863b95 49 minutes ago 85.8MB ```
|
Thanks, @camerondurham! This looks great. Getting
v1.56 is fine, though as of today there's a v1.56.1.
I think it's worth mentioning in the |
| RUN apt-get update -y \ | ||
| && apt-get install -y ${builddeps} \ | ||
| && git submodule update --init --recursive | ||
| RUN cargo install --path . |
There was a problem hiding this comment.
It'd be nice to make sure this configuration works each time we commit new code. How easy would it be to add building this Docker image to the GitHub actions?
There was a problem hiding this comment.
Agreed, this should definitely have an action.
| # mount current directory to /data volume and dump an ion file | ||
| docker run -it --rm -v $PWD:/data ion-cli:0.1.1 ion dump /data/test.ion |
There was a problem hiding this comment.
Thanks for including this example, it's really helpful. 👍
|
Thanks for the review and comments @zslayton ! Revision summary:
On my fork, the workflow is working as expected. Summary in repo commits here:
I am sure you are aware of this, but this repo's action policy should maybe limit when actions run for certain contributors if it isn't already? Docs: Configuring Required Approval For Workflows |
zslayton
left a comment
zslayton
left a comment
There was a problem hiding this comment.
This looks great, thanks for contributing!
| This will put a copy of the `ion` executable in `~/.cargo/bin`. | ||
|
|
||
| **If this step fails:** You're likely missing one of `ion-c`'s dependencies. Make sure you have `cmake`, `gcc`, `g++`, and `libc++` installed. | ||
| **If this step fails:** You're likely missing one of `ion-c`'s dependencies. Make sure you have `cmake`, `gcc`, `g++` and `clang` installed. On Debian-based Linux distributions, the only required dependencies are `cmake` and `clang`. |
There was a problem hiding this comment.
Something about this isn't quite right; we shouldn't require both gcc and clang. I don't want this to block the PR, though, so I'll open an issue to track it.
There was a problem hiding this comment.
Agreed, that doesn't seem right. I did only require clang for the Debian build but hadn't done much work to know if gcc was needed when building on macOS/Windows.
2 participants
Hi there!
I've added a Dockerfile that makes a relatively small multi-stage image of the CLI that should be usable cross-platform (only tested on macOS so far but no reason to believe it would fail on Windows/Linux).
I'm interested in contributing something more meaningful (i.e. tests for #10 when I have a little more time) but wanted to send this for now. I also understand if you don't want to merge since it does sadly change your repo stats from 100% to 99.1% Rust, 0.9% Dockerfile 😄.
Question for maintainers:
cmakeandclang. Should I update the README to reflect this or Dockerfile be sufficient?Thanks!
Issue #, if available:
N/A
Description of changes:
Recording of local build/test:
Currently builds image size 85.8MB from local build on macOS
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.