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

Support configuring the OCAML version #10

Merged
merged 3 commits into from
Jan 20, 2020
Merged

Conversation

twodrops
Copy link
Contributor

The current docker-image-unison relies on OCAML versions available within Alpine. Just like a Unison version could be chosen, it would be nice if OCAML versions could be chosen too.

I had to use a OCAML 4.06.1 and Unison 2.51.2 which is not possible with the current image.

The PR compiles the chosen OCAML version and uses it for compiling unison as before. The size of the image is also reduced from the earlier 485MB to 280MB. The disadvantage is of course the increased build time of the image because of the time required to compile OCAML.

@twodrops twodrops changed the title Support configuring a the OCAML version Support configuring the OCAML version Jan 19, 2020
@EugenMayer
Copy link
Owner

Sounds fairly reasonable and the one time build time does not really counter image size and flexibility. Question is though, should we offer different builds+tag those, it stick the official with latest ocalm?

@twodrops
Copy link
Contributor Author

I guess we could stick with the latest OCAML as it is now. If someone needs, they could always pass the argument and build a new one. On the host side, this is already done.
As several combinations of OCAML and Unison are possible, maintaining several builds will be tedious. In such case it would be better to provide OPAM in the image so that the users switch it as they need. I have a second Dockerfile with OPAM. As the image size including OPAM was higher, I decided to go for this one.

@EugenMayer
Copy link
Owner

I understand.

Looking at the Dockerfile, i see that you kept the default as it was, that's great. Could you add some lines to the readme to let people know they can change the OCALM version and a short hint how ( i understand this is basic docker foo, but still, this can help )

@twodrops
Copy link
Contributor Author

Could you add some lines to the readme to let people know they can change the OCALM version and a short hint how ( i understand this is basic docker foo, but still, this can help )
Sure. Just did that. Please have a look.

@EugenMayer
Copy link
Owner

This is even better then i expected / asked for, thay you a lot. LGTM and thank you for the lovely contribution!

@EugenMayer EugenMayer merged commit c07d645 into EugenMayer:master Jan 20, 2020
@twodrops
Copy link
Contributor Author

Short question: Do you plan to push this to docker hub? I guess you will then need to update docker-sync to reference the new image instead of current eugenmayer/unison:2.51.2.2 right? For users of docker-sync the new image will not directly bring any benefits (other than small image size). Just that, the ones who will host they own image, will end up having a different base image than the current default one.

If you release, please do a testing from you side with the new image.

@EugenMayer
Copy link
Owner

I have thought about this but decided to rather not do so since as you explain, size is the only thing that would bring benefits.

While on the other side, the compiled unison version in the past had a r1 suffix, now it does not. And since they have to match I fear regression bugs on some systems where this becomes an glitch.

Sure, we could just test all this out but yet it seems the effort might not bring anything to the table.

I think the real value here is for users to build custom images and push them on hub for the team for custom OCalm support.

Do you think differently?

@twodrops
Copy link
Contributor Author

I agree completely. I have done the same by hosting an image based on Unison 4.06.1 internally.
Do you see a violation of GPL here? Earlier I was modifying your GPL code, now with the argument support I am only rebuilding it.

@EugenMayer
Copy link
Owner

Every person who gets the image should be able to access the image code - and since this is here in the public it is valid for every person you share it with - no GPL violation IMHO, but take me not as a legal advice :)

You even contributed back so there is really no argument to build here, I would never try to do that at least :)

@EugenMayer EugenMayer mentioned this pull request Apr 25, 2020
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.

None yet

2 participants