-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add Dockerfile and instructions #13
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks for contributing!
@@ -24,7 +24,7 @@ and the API is subject to change._ | |||
``` | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #14.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
cmake
andclang
. 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.