-
Notifications
You must be signed in to change notification settings - Fork 6
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
Makefile: Add deps-ubuntu target #12
Conversation
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.
Great idea!
(Must also adapt for that manually in ocrd_all afterwards.)
Co-authored-by: Robert Sachunsky <[email protected]>
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!
I will also open a pull request on ocrd-all when this get merged. |
Splendid. If you do, just add a line… deps-ubuntu: ocrd_pagetopdf …above the existing rules for ocrd-pagetopdf. |
Makefile
Outdated
@echo "" | ||
@echo " Variables" | ||
@echo "" | ||
@echo " PREFIX Directory to install to ('$(PREFIX)')" | ||
|
||
# END-EVAL | ||
|
||
# Install system packages (on Debian/Ubuntu) | ||
deps-ubuntu: | ||
apt-get install -y make python3 python3-pip python3-venv openjdk-8-jre-headless ghostscript |
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.
How would I run this rule if make
was not already installed?
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.
True, make
is redundant at this point.
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.
Nor do we need python3-pip, as pip
gets installed with a Python3 venv.
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.
Otherwise these changes are fine, thank you. Please consider my suggested change, and if it works for you, we can merge this pull request.
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.
make
dependency inside the makefile is unnecessary, otherwise LGTM
@kba, can I merge with my changes? |
Remove the unneeded packages `make` (already required earlier) and `python3-pip` (not needed when using venv).
@sulzbals, thank you for your contribution. |
@echo "" | ||
@echo " Variables" | ||
@echo "" | ||
@echo " PREFIX Directory to install to ('$(PREFIX)')" | ||
|
||
# END-EVAL | ||
|
||
# Install system packages (on Debian/Ubuntu) | ||
deps-ubuntu: | ||
apt-get install -y python3 python3-venv openjdk-8-jre-headless ghostscript |
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.
Would openjdk-11-jre-headless
also work? Newer Linux distributions don't provide JRE 8.
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.
No, the PRImA libs still require JRE 8 AFAIK (dimly recall getting that answer directly from Christian Clausner somewhere). Unfortunately, build and deps are not well documented for PRImA's projects, but you can open an issue about it on https://github.com/PRImA-Research-Lab/prima-core-libs or https://github.com/PRImA-Research-Lab/prima-page-converter.
Add target for installing the system dependencies listed in the README on a Debian/Ubuntu system.
Follows the same naming format of other ocrd modules (e.g. core), and could be used by ocrd-all when installing this module (I had to install
openjdk
manually after installing this with ocrd-all).