Fix: 1257 - Load ocrd tool json locally

[MehmedGIT]

A fix for #1257.

[bertsky]

Straightforward.

But then perhaps we should make sure that an ocrd-all-tool.json does always exist in any given core installation, independent of our ocrd_all recipes. We could create a default ocrd-all-tool.json (with just the ocrd-dummy processor) as package data. (Which the ocrd_all recipes would merely overwrite in turn.)

[bertsky]

Oh, the integration tests fail here:

Container ocrd_network_processing_server  Starting
 Container ocrd_network_processing_server  Started
 Container ocrd_network_processing_server  Waiting
 Container ocrd_network_processing_server  Waiting
 Container ocrd_network_processing_server  Error
 Container ocrd_network_processing_server  Error
dependency failed to start: container ocrd_network_processing_server exited (1)
make: *** [Makefile:293: network-integration-test-cicd] Error 1

With no further messages, I suppose this will be hard to reproduce, right?

[MehmedGIT]

Oh, the integration tests fail here:

Container ocrd_network_processing_server  Starting
 Container ocrd_network_processing_server  Started
 Container ocrd_network_processing_server  Waiting
 Container ocrd_network_processing_server  Waiting
 Container ocrd_network_processing_server  Error
 Container ocrd_network_processing_server  Error
dependency failed to start: container ocrd_network_processing_server exited (1)
make: *** [Makefile:293: network-integration-test-cicd] Error 1

With no further messages, I suppose this will be hard to reproduce, right?

The reason behind this is the missing ocrd-all-tool.json file.

[bertsky]

The reason behind this is the missing ocrd-all-tool.json file.

Oh, of course!

Then I suggest modifying this PR to include the default ocrd-all-tool.json (with just the dummy processor, which should suffice for dummy-workflow.txt in the integration test) file as package already.

[MehmedGIT]

The reason behind this is the missing ocrd-all-tool.json file.

Oh, of course!

Then I suggest modifying this PR to include the default ocrd-all-tool.json (with just the dummy processor, which should suffice for dummy-workflow.txt in the integration test) file as package already.

Done. Right before your comment, I have also added a download if it is missing.

[bertsky]

Done.

Ah, thanks.

I was just thinking about generating this dynamically – basically a json.dump(json.load(open('ocrd-tool.json'))['tools'], 'ocrd-all-tool.json') hooked into cmdclass.build of setup.py...

Right before your comment, I have also added a download if it is missing.

Ok, so we have both mechanisms. I wonder if it wouldn't be better to not support the download strategy anymore...

BTW, now a new CI failure appeared: we get interference from some new paramiko version's stderr chatter about coming deprecations (completely irrevelant to us). Perhaps we can suppress this within our ocrd_logging.conf?

[MehmedGIT]

BTW, now a new CI failure appeared

Setting paramiko and paramiko.transport logging to ERROR does not help. I cannot reproduce the error locally, tests pass successfully. The order of the tests seems to mess that up. Changing the file tests/cli/test_bashlib.py to tests/cli/test_processor_bashlib.py should also fix the issue. Even better solution might be writing the output to a file, and reading from the file to verify the output instead of relying on stdout and stderr. There is no new paramiko version. The latest is 3.4.0 and we are still using that version.

[bertsky]

Setting paramiko and paramiko.transport logging to ERROR does not help.

Indeed – they use the warnings facility instead of logging. See paramiko/paramiko#2419 for a workaround based on a warnings filter.

I cannot reproduce the error locally, tests pass successfully.

It will depend on the venv's (or OS'?) cryptography version.

Changing the file tests/cli/test_bashlib.py to tests/cli/test_processor_bashlib.py should also fix the issue. Even better solution might be writing the output to a file, and reading from the file to verify the output instead of relying on stdout and stderr.

Right – testing for zero stderr was not a good pattern in the first place.

[kba]

LGTM. Pinning cryptography to an older version is problematic but if paramiko/paramiko#2421 is merged soon, there will probably a proper solution soon.

Ready for merge?