Conditional modules and editable mode installation #24
Conversation
Makefile
Outdated
|
|
||
| .PHONY: ocrd | ||
| ocrd: $(BIN)/ocrd | ||
| $(BIN)/ocrd: core | ||
| . $(ACTIVATE_VENV) && cd $< && make install PIP_INSTALL="$(PIP_INSTALL) --force-reinstall" | ||
| . $(ACTIVATE_VENV) && $(MAKE) -C $< install PIP_INSTALL="$(PIP) install --force-reinstall $(PIP_OPTIONS)" |
There was a problem hiding this comment.
Those --force-reinstall options are sometimes nagging. I'd like an option to disable them in local.mk (after this PR was merged).
There was a problem hiding this comment.
Sure! We could wrap them in a variable with install --force-reinstall that you can then override.
But I'm curious: what problems do they give you? I introduced this to ensure the executable files we use as targets always get updated along with new module versions (even if the new version did not directly affect them). So if you remove this option, make will attempt re-building them every time.
bertsky
force-pushed
the
conditional-modules
branch
from
178cf2e
to
c721848
Compare
|
@kba, if you want I can add 2 commits that remove ocrd_kraken and ocrd_ocropy now. Oh, and please tag this repo with the topic |
bertsky
force-pushed
the
conditional-modules
branch
from
a0c69f0
to
ec4f7a0
Compare
|
So, any more suggestions/objections, or can I merge? |
bertsky
force-pushed
the
conditional-modules
branch
from
ec4f7a0
to
16740c5
Compare
Thanks @stweil! I was sloppy with the module updates (ocrd_cis uses/needs the |
- allow overriding OCRD_MODULES to select only a subset of
git submodules for all targets:
- only look for git submodules if OCRD_MODULES is empty or unset
- exclude all rules and variables of deactivated modules
(in module-local scope with conditionals)
- better encapsulation for `deps-ubuntu`:
- add modules with local `deps-ubuntu` as prerequisites directly
instead of aggregating their names in a list
- aggregate packages for other modules in a list
(in module-local scope)
- avoid installing Tesseract from both source and PPA at the same time:
- add `install-tesseract` to `all` if `tesseract` module is active
- use `deps-ubuntu` from `ocrd_tesseract` iff `tesseract` module is inactive
- add `libleptonica-dev` if `tesseract` module is active
- try to get similar sets of Tesseract languages/models,
regardless of whether installing from source or PPA:
use a single variable TESSERACT_MODELS for both download and apt-get
(variable can be overridden as usual, and triggering downloads
manually/selectively remains possible, too)
- provide `clean-tesseract` and `clean-olena`,
make `clean` depend on them
- allow pip installation in editable mode:
- allow adding `-e` to `pip install` options
- split up PIP_INSTALL into PIP and PIP_OPTIONS
(because `-e` must always be last if present, and must only apply
to source directories, not mere package names)
- simplify docker build, and add preconfigured selections:
- use new OCRD_MODULES filtering instead of parsing dependencies
for DOCKER_MODULES
- use wildcard on `.git` instead of `git init` to avoid submodule updates
(more robust, also works with editable mode)
- when installing in editable mode, don't remove the build directory
(which now contains the only copy of the modules),
but only clean its build directories
- be more selective in workaround for ImageMagick PDF/PS security restriction
- besides `docker` (now) with user selection of OCRD_MODULES,
add target `dockers` dependent on 3 new preconfigured subsets of OCRD_MODULES:
`docker-minimum` / `docker-medium` / `docker-maximum`
- make editable mode the default for these unless DOCKERS_WITHOUT_REPOS is set:
`docker-minimum-git` / `docker-medium-git` / `docker-maximum-git`
- add a subtag for all Docker images based on their target name
(e.g. use `ocrd/all:medium-git` for docker-medium-git, but keep
`:latest` when target was just `docker`)
- include `local.mk` if present to allow persisting variable choices
(or any other customization) across invocations
- update README to reflect above changes
- (during rebase:) use correct module name for ocrd_pc_segmentation
Co-Authored-By: Stefan Weil <[email protected]>
Co-Authored-By: Stefan Weil <[email protected]>
…d process Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
stweil
force-pushed
the
conditional-modules
branch
from
16740c5
to
a0ab93a
Compare
|
@bertsky, I rebased your commits and added some more commits. Like that, I think it is ready for merging. Please review my additions. |
Signed-off-by: Stefan Weil <[email protected]>
stweil
force-pushed
the
conditional-modules
branch
from
a0ab93a
to
dc1d462
Compare
|
|
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
There was a problem hiding this comment.
@stweil These changes break my uniform solution for both wget (for local build) and apt (for PPA install). I cannot even request changes here, because:
Pull request authors can't request changes on their own pull request.
Why didn't you first discuss this with me? If you wanted to keep a minimum list, not overridable by the user, this could have been done much simpler (and without breaking uniformity with the PPA method).
|
The last 2 changes are ok of course. (Strange that the autoconf manual is incorrect and you still need |
Fixed that now. @stweil, your review status still says |
That was because of the broken default help goal (which I fixed later in a commit without updating the status). |
This is another major leap forward. It encapsulates knowledge about submodule specifics even better (esp. system dependencies) and allows selectively factoring modules out of all targets (including
deps-ubuntuanddocker). It also allows installing pip packages in editable mode (to save space and enable faster update regimes, both especially helpful in Docker images). Lastly, it adds support for a (non-git controlled) local/site customization makefile.@stweil, this also avoids installing Tesseract both from source and PPA at the same time.
@kba, this also contains preconfigured Docker builds of different extent (which you could offer on an automatic basis via Dockerhub).
I did 2dfe06a in light of #15, but we probably should find a better way to fix the issue before merging.
Please don't ask me to split 009a042 into multiple commits – it's hard and would not make the changes more comprehensible at all (due to interdependencies). I'll gladly elaborate on any individual change here, though.