-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: [WIP] fix blockers for podman compatibility #110
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #110 +/- ##
==========================================
+ Coverage 68.51% 68.59% +0.07%
==========================================
Files 26 26
Lines 2547 2547
Branches 384 384
==========================================
+ Hits 1745 1747 +2
+ Misses 656 654 -2
Partials 146 146 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@jpbrodrick89 I did not include the |
Yes, that's right. We are discussing this in #104 so happy for you to leave off as we can address there. Addressing this is not required to run with Podman and I don't seem to actually need the base image. Glad the changes didn't fail any tests (had to rerun Windows though for some reason!). |
@@ -70,6 +68,9 @@ ENV TESSERACT_NAME="{{ config.name }}" \ | |||
COPY --from=build_stage /python-env /python-env | |||
COPY "{{ tesseract_source_directory }}/tesseract_api.py" ${TESSERACT_API_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.
fyi, you can use COPY --chown=tesseractor:tesseractor
here instead of running chown
separately on the file.
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, we'll need to do the same for copying across extra_packages
which was required to get the capsule optimisation file working.
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.
@dionhaefner thanks, that's a good tip. do you prefer doing it file-by-file instead of chown -R
on the whole directory at the end? I'm happy with either approach.
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.
Yes, I believe this is a best practice that has slightly more well-behaved Docker caching and whatnot.
Including these changes in #112, let me know in case there's more you want to get in. |
Relevant issue or PR
On the path to resolving #8.
We can merge this if the changes are considered unoffensive, or we can wait until #33 lands to verify that the changes in here fix an error when building and running with podman.
Description of changes
local_requirements
directory even in the case where there are none; in this case it will be empty. This is required becausepodman
errors on theCOPY local_requirements* ./local_requirements
line if the directory doesn't exist.chown -R
after copyingtesseract_api.py
into/tesseract
. This fixes a permissions error on that file when built bypodman
.Testing done
These changes were arrived at during the process of figuring out what's necessary to build and run a tesseract on the NERSC perlmutter HPC cluster.
License
Signed-off-by
line.Developer Certificate of Origin
Signed-off-by: Jack Coughlin [email protected]