-
Notifications
You must be signed in to change notification settings - Fork 19
[no ci] [RFC] Rework build process #295
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
Conversation
|
I wonder whether we really need |
We don't need it any more, we did need it when we still had to support 2.7 under certain circumstances. In fact, using anything but |
|
Now 3 of 5 Python versions complete the build: https://github.com/stweil/ocrd_all/actions/runs/1993530591. The remaining failures occur when building |
| ACTIVATE_VENV = $(VIRTUAL_ENV)/bin/activate | ||
|
|
||
| ifeq (0, $(MAKELEVEL)) | ||
| ifeq ($(MAKECMDGOALS), all) |
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.
TODO: support also builds with several targets, for example make all check.
| ifeq ($(MAKECMDGOALS), all) | ||
| CHECK_SUBENVS := true | ||
| endif | ||
| ifeq ($(MAKECMDGOALS), check) |
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.
TODO: support also builds with several targets, for example make all check.
| create_venv := $(shell $(PYTHON) -m venv $(SUB_VENV)/headless-tf21 && bash -c "source $(SUB_VENV)/headless-tf21/bin/activate && pip install -U pip setuptools wheel") | ||
| endif | ||
|
|
||
| # Try to install different versions of Tensorflow. |
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.
TODO: This takes some time when the installation is really done (or tried), so a log message would be useful.
|
Now all builds pass. |
|
The updated branch still passes for all builds: https://github.com/stweil/ocrd_all/actions/runs/2027885009. |
31335e4 to
3d2605a
Compare
| else | ||
| cd $< ; $(MAKE) patch-pix2pixhd | ||
| # Hack for Python 3.10 which fails to install ocrd-fork-pylsd 0.0.4 from PyPI. | ||
| . $(ACTIVATE_VENV) && pip install git+https://github.com/kba/pylsd.git |
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.
@kba, @bertsky, could you have a look why pip3.10 install ocrd-fork-pylsd fails while pip3.10 install git+https://github.com/kba/pylsd.git works fine? I get this build error:
gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -Isource/include -I/home/stweil/src/github/OCR-D/venv3.10-20220323/include -I/usr/local/include/python3.10 -c source/src/lsd.cpp -o build/temp.linux-x86_64-3.10/source/src/lsd.o
source/src/lsd.cpp:77:10: fatal error: lsd.h: No such file or directory
77 | #include <lsd.h>
| ^~~~~~~
compilation terminated.
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.
I tried to build a manylinux binary wheel for 3.10. Can you try
pip install --only-binary :all: ocrd-fork-pylsd
and see if that works for you?
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.
That works, and pip3.10 install ocrd-fork-pylsd works now, too.
|
Latest run passes all builds: https://github.com/stweil/ocrd_all/actions/runs/2029562843. |
e5db111 to
dcdcfb2
Compare
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
…f missing tensorflow Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
They are required because of new package conflicts for Python < 3.10. Signed-off-by: Stefan Weil <[email protected]>
|
@bertsky, @kba, I compared the build rules here with those from git master and wrote the complete |
|
Now I also have timing and size results for a local |
|
This proof of concept is no longer up to date, so it can be closed. |
I currently try to rework the build process to support more modern Linux distributions and Python versions. Ideally the full test matrix of Ubuntu LTS versions and Python versions ranging from 3.6 to 3.10 should build fine.
My changes address several issues:
ocrd_kraken.ocrd_cisunconditionally because it requires an oldcalamari_ocrcausing version conflicts.wheelearly in all virtual environments, so it is no longer needed as a dependency.This PR is not for merging, but for discussion of the different aspects with the goal to find a consensus on the right solution.