-
Notifications
You must be signed in to change notification settings - Fork 14
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 Makefile to create ocr-d dockerimage #120
Conversation
I'm all for it, have just been waiting for some kind of OCR-D/ocrd_all "standard" to follow.
Frankly, I'm not a big fan of "Makefiles everywhere", but I also don't see any better solution, so that's OK :) Containerization is definitely wanted.
Could you elaborate on that? Maybe we can clear it up.
dinglehopper has two CLIs: |
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.
Leaving it a "Request changes" for now, but it's only small stuff.
Another question: Should dinglehopper's CI build the image or will that be done by e.g. ocrd_all? |
Preferably via local CD here. See here for an example GHA CD which pushes to both Dockerhub and GHCR. (All it takes is to set up a |
Co-authored-by: Robert Sachunsky <[email protected]>
I fancy-clicked @bertsky's change suggestion, which duplicated some labels. Now fancy-clicking the fix, fingers crossed...
I've opened #121. Any reasoning behind pushing to two container registries? |
So, then all that's left to clear up is:
|
Thanks both of you for extending this PR. I'll mark it ready to merge then.
Sorry for the confusion. The project layout is totally fine and working for ocr-d purposes, it just made me wonder if this PR would be accepted. My intention was simply to not make a PR and expecting it to be merged with stuff which may not be wanted by the maintainer (you). But I see it's ok with you. |
Alright! I'll give the Docker image a spin and if it looks good, I'll merge. (I opened a separate issue for the CD builds.) I might do something slightly differently (e.g. |
In the current state it executes |
I tested the built image superficially, looks OK! |
Yes, that's inherited from the previous stage. I don't know why we put the |
This PR adds a Dockerfile and Makefile to create a dockerimage for this processor. Ideally all OCR-D processors offer the same way to create an image for them, which is currently missing in this repo.
I am not sure though if this is desired here as well, because it seems to me dinglehopper has slightly other layout as a "normal" ocr-d processor has. Dinglehopper seems also to be supposed to be used detached from ocr-d. That's why I created a draft PR at first.