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

sub-venv rules: move core dependency to inner make exclusively #217

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

bertsky
Copy link
Collaborator

@bertsky bertsky commented Nov 12, 2020

This fixes a problem I've overlooked initially: Having the ocrd (or core) dependency above the MAKELEVEL distinction, i.e. visible both on the inner and outer invocation, is not only unnecessary (as the outer rules strictly only need a bash script), but may break when the outer venv is not fully installed already (which may even happen on an initial checkout when running make -j all). Example:

make ocrd-anybaseocr-crop -W core
. /tmp/ocrd_all/venv/local/sub-venv/headless-tf21/bin/activate && make -C core install PIP_INSTALL="pip3 install "
/bin/sh: 1: .: Can't open /tmp/ocrd_all/venv/local/sub-venv/headless-tf21/bin/activate
Makefile:162: recipe for target '/tmp/ocrd_all/venv/bin/ocrd' failed

Explanation: the outer $(BIN)/ocrd rule gets checked for dependencies in a context where all immediate variables are expanded (make phase 1), with the outer value for VIRTUAL_ENV. So if the outer venv does not exist yet, it gets created. But then that rule's recipe gets executed in a context where all deferred variables are expanded (make phase 2), with the inner value of the sub-venv. But since we are still in the outer make level, we have no rules to create the inner venv yet.

This PR therefore moves all these dependencies into the inner context exclusively.

@kba kba merged commit 1028516 into OCR-D:master Nov 16, 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