Skip to content

Conversation

@tmathern
Copy link
Collaborator

  • Update c2pa.py with latest changes from the POC branch
  • Update makefile
  • Clean up tests so resources are properly released

@tmathern tmathern requested a review from gpeacock May 14, 2025 20:40
@tmathern tmathern self-assigned this May 14, 2025
tmathern added 2 commits May 14, 2025 14:20
* fix: Some renaming going on

* fix: Format

* fix: Add scripts to debug builds, and clean env

* fix: gitgnore

* fix: Update makefile comments

* fix: verbose debug logs

* fix: Only keep the scripts we need

* fix: Improve makefile
@tmathern tmathern requested a review from gpeacock May 14, 2025 22:02
REPO_NAME = "c2pa-rs"
GITHUB_API_BASE = "https://api.github.com"
ARTIFACTS_DIR = Path("artifacts")
SCRIPTS_ARTIFACTS_DIR = Path("scripts/artifacts")
Copy link
Collaborator Author

@tmathern tmathern May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can run this from the root of the repo. I used this instead of build.py because build.py seems broken, whereas that one works. Let me know if build.py was meant to stay or not - but then it likely needs debugging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did work on build.py to get it functional, but I can merge that in.

@tmathern tmathern dismissed gpeacock’s stale review May 15, 2025 18:40

Can you re-review?

Copy link
Contributor

@gpeacock gpeacock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot here. I added some feedback. Let me know what you think.

REPO_NAME = "c2pa-rs"
GITHUB_API_BASE = "https://api.github.com"
ARTIFACTS_DIR = Path("artifacts")
SCRIPTS_ARTIFACTS_DIR = Path("scripts/artifacts")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did work on build.py to get it functional, but I can merge that in.

if not result: # NULL pointer
if check_error:
_handle_c2pa_error()
error = _lib.c2pa_error()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels odd to me. handle_string_result is designed specifically to return string results on methods that return strings. Using it everywhere when you already know you want to return an error is confusing. why not just call handle_c2pa_error if that is what you know you are doing?

Copy link
Collaborator Author

@tmathern tmathern May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no handle_c2pa_error in that branch... ? Maybe in your branch... (I'll check and update if I see it there (not found?))? And I keep having to check _lib.c2pa_error() to not miss any error (remember we had the case where an error string would be reported there, but raise no error, or the called function's result would report being ok...). This is the additional layer to not miss any error with the current setup.

Copy link
Collaborator Author

@tmathern tmathern May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed _parse_operation_result_for_error but I am not totally sure we both aim for the same thing here. I aim to throw errors when errors happened (what _parse_operation_result_for_error now does), but you want to return strings? Returning strings may be an unexpected way to handle errors for people in Python more accustomed to dealing with exceptions being thrown.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely want to throw errors when they happen! I created a bunch of odd little macros to handle things reliably in the Rust C binding logic. They are oddly named and I need to document them and explain how they work. I've tried to follow old school C error handling in the C APIs, but it needs some work. Ideally every function would return an indication if there is an error or not, either by returning a negative number or returning null. If there's an error, we would always read the last error message and then throw a constructed error in the higher level language. The error string returned by c2pa_error is formatted to included an error name followed by a colon. The rest of the string contains the error message. The error name can be converted into a specific kind of C2paError in languages like Python.
I know there some inconsistencies and I've been trying to eliminate them. I want all possible null parameters checked and a consistent policy on setting the last error so that we never end up with an empty error state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. In that case, I would suggest to merge that PR into your branch, and rework from there (I really don't want the 2 branches to diverge too much).

Copy link
Contributor

@gpeacock gpeacock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's merge this and we can rework things in one place

@tmathern tmathern merged commit c68399c into gpeacock/no_rust May 29, 2025
2 of 15 checks passed
@tmathern tmathern deleted the mathern/python-only branch May 29, 2025 20:20
tmathern added a commit that referenced this pull request Jun 16, 2025
* feat: Rewrite this repo to use pre-built c2pa_rs libraries and no direct Rust compilation

* fix: Add sdk_version and get the current value from version()

* build: work in progress on build updates

* feat: Update c2pa.py (#105)

* fix: Library loader

* fix: Refactor

* fix: Loader

* fix: Clean up tests

* fix: Some renaming going on (#106)

* fix: Simplify build (#107)

* fix: Some renaming going on

* fix: Format

* fix: Add scripts to debug builds, and clean env

* fix: gitgnore

* fix: Update makefile comments

* fix: verbose debug logs

* fix: Only keep the scripts we need

* fix: Improve makefile

* chore: rename 1 function

* fix: Rename errors camel case

* fix: Add debug logs to the loading of libs

* fix: Format download logs

* fix: Platform checks, download only one platform

* fix: Load library, one test failure

* feat: Add autopep8 formatting

* fix: Dev rebuild

* fix: Install package from local

* fix: Temp deactivation of tests to debug publishing

* fix: split install and wheel build step

* fix: Debug Linux build

* fix: Simplify deps graph

* fix: Source dist

* fix: Fix warning

* fix: Restore macos_x86

* fix: Restore macos_aarch64

* fix: Restore windows step

* fix: Introduce reusable workflow

* fix: Permissions

* fix: restore tests step

* fix: Pytest is touchy about venv

* fix: Missing deps

* fix: split out windows tests

* fix: split out windows tests 2

* fix: Rename steps

* fix: Rename the steps

* fix: Test graph

* fix: name of actions

* fix: Missing params

* fix; conditional check

* fix: auth requests to avoid rate limtis

* fix: Dirs

* fix: test wheel

* fix: Just build

* fix: Test wheels

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Version number check

* fix: Version mismatch issue

* fix: Rename wheels

* fix: Build change

* fix: follow-up text processing fix

* fix: Drop bdist_wheel use

* fix: Missed a spot

* fix: License inclusion in wheel tweaks

* fix: Add back the deny.toml file

* fix: sdist build

* fix: Clean up deps

* fix: setuptools bug handling

* fix: Better makefile

* fix: deps oversight

* fix: Split deps

* fix: Space typo

* fix: bump version

* fix: Upload

* fix: wheel upload debug

* fix over-eager version up

* fix over-eager version up

* fix over-eager version up

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: fix warning

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* feat: manylinux support (#109)

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Update test setup

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Fix setups once more (#110)

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Update test setup

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Test

* fix: Add test

* fix: Refactor tests

* fix: Reader test

* fix: Update readme

* fix: Docs

* fix: Refactor

* fix: benchmark

* fix: CLeaning up

* fix: Stream tests

* fix: Reader tests

* fix: Builder double close test

* fix: Clean up

* fix: Clean up

* fix: Remove debug logs

* fix: Fix typo

* fix: Stream exception

* fix: docs & more tests (#111)

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Update test setup

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Test

* fix: Add test

* fix: Refactor tests

* fix: Reader test

* fix: Update readme

* fix: Docs

* fix: Refactor

* fix: benchmark

* fix: CLeaning up

* fix: Stream tests

* fix: Reader tests

* fix: Builder double close test

* fix: Remove debug logs

* fix: Fix typo

* fix: Stream exception

* fix: Threaded reading

* fix: Threading tests

* fix: Interleaved thread test

* fix: Threading

* fix: More tests

* fix: Test updates

* fix: Test updates

* fix: Add some async tests

* fix: Test updates

* fix: Test updates

* fix: Test updates

* fix: Test updates

* fix: Add ingredients tests

* fix: Test ingredient

* fix: Test ingredient

* fix: Add ingredient from stream

* fix: Ingredient from stream

* fix: Reorder test

* fix: Thread

* fix: Threading with asyncio

* fix: RUn all the tests

* fix: refactor tests

* fix: refactor and format

* fix: refactor and format 2

* fix: benchmark

* fix: FOrmat

* fix: Add test for contention

* fix: Add test for contention 2

* fix: Threads

* fix: Paths use and some timing

* fix: autopep8

* fix: Format

* fix: Undo test formating as json formatting seems touchy

* fix: test_read_ingredient_file

* fix: format

* fix: format

* fix: fix a bug found by tests

* fix: Refactor sign_file to use new API internally, but keep the emthod for compatibility

* fix: Clean up

* fix: CLean up

* fix: Remove debug code

* fix: Ignore deprecation warnings in test logs

* fix: One more clean up

* fix: One more clean up

* feat: Prepare publishing flow to PyPi, increase test coverage (#108)

* fix: Dev rebuild

* fix: Install package from local

* fix: Temp deactivation of tests to debug publishing

* fix: split install and wheel build step

* fix: Debug Linux build

* fix: Simplify deps graph

* fix: Source dist

* fix: Fix warning

* fix: Restore macos_x86

* fix: Restore macos_aarch64

* fix: Restore windows step

* fix: Introduce reusable workflow

* fix: Permissions

* fix: restore tests step

* fix: Pytest is touchy about venv

* fix: Missing deps

* fix: split out windows tests

* fix: split out windows tests 2

* fix: Rename steps

* fix: Rename the steps

* fix: Test graph

* fix: name of actions

* fix: Missing params

* fix; conditional check

* fix: auth requests to avoid rate limtis

* fix: Dirs

* fix: test wheel

* fix: Just build

* fix: Test wheels

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Debug build

* fix: Version number check

* fix: Version mismatch issue

* fix: Rename wheels

* fix: Build change

* fix: follow-up text processing fix

* fix: Drop bdist_wheel use

* fix: Missed a spot

* fix: License inclusion in wheel tweaks

* fix: Add back the deny.toml file

* fix: sdist build

* fix: Clean up deps

* fix: setuptools bug handling

* fix: Better makefile

* fix: deps oversight

* fix: Split deps

* fix: Space typo

* fix: bump version

* fix: Upload

* fix: wheel upload debug

* fix over-eager version up

* fix over-eager version up

* fix over-eager version up

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: fix warning

* fix: upload of wheels

* fix: upload of wheels

* feat: manylinux support (#109)

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Fix setups once more (#110)

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Update test setup

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Clean up

* fix: Clean up

* fix: docs & more tests (#111)

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Update test setup

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Test

* fix: Add test

* fix: Refactor tests

* fix: Reader test

* fix: Update readme

* fix: Docs

* fix: Refactor

* fix: benchmark

* fix: CLeaning up

* fix: Stream tests

* fix: Reader tests

* fix: Builder double close test

* fix: Remove debug logs

* fix: Fix typo

* fix: Stream exception

* fix: Remove debig code

* fix: Deps

* feat: Simplify version number handling (#113)

* feat: Improve version numbers handling

* fix: Test build

* fix: Test build

* fix: Test build

* fix: Test build

* fix: Test build

* fix: Test build

* fix: Test build debug

* fix: Test build debug

* fix: Test build debug

* fix: Test build debug

* fix: Test build debug

* fix: Test build debug

* fix: Test build debug

* fix: Test build debug

* fix: Test build debug

* fix: Test build debug

* fix: fix makefile typo

* fix: Test build debug

* fix: Change build script

* fix: Test build debug

* fix: Test build debug

* feat: Add (more) unit tests (#112)

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Update test setup

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: upload of wheels

* fix: Test

* fix: Add test

* fix: Refactor tests

* fix: Reader test

* fix: Update readme

* fix: Docs

* fix: Refactor

* fix: benchmark

* fix: CLeaning up

* fix: Stream tests

* fix: Reader tests

* fix: Builder double close test

* fix: Remove debug logs

* fix: Fix typo

* fix: Stream exception

* fix: Threaded reading

* fix: Threading tests

* fix: Interleaved thread test

* fix: Threading

* fix: More tests

* fix: Test updates

* fix: Test updates

* fix: Add some async tests

* fix: Test updates

* fix: Test updates

* fix: Test updates

* fix: Test updates

* fix: Add ingredients tests

* fix: Test ingredient

* fix: Test ingredient

* fix: Add ingredient from stream

* fix: Ingredient from stream

* fix: Reorder test

* fix: Thread

* fix: Threading with asyncio

* fix: RUn all the tests

* fix: refactor tests

* fix: refactor and format

* fix: refactor and format 2

* fix: benchmark

* fix: FOrmat

* fix: Add test for contention

* fix: Add test for contention 2

* fix: Threads

* fix: Paths use and some timing

* fix: autopep8

* fix: Format

* fix: Undo test formating as json formatting seems touchy

* fix: test_read_ingredient_file

* fix: format

* fix: format

* fix: fix a bug found by tests

* fix: Refactor sign_file to use new API internally, but keep the emthod for compatibility

* fix: Clean up

* fix: CLean up

* fix: Remove debug code

* fix: Ignore deprecation warnings in test logs

* fix: One more clean up

* fix: One more clean up

* fix: Crypto up

* fix: Readme

* fix: update examples

* fix: Examples

* fix: Run examples as parts of tests

* fix: undo previous change, but keep makefile helper

* fix: Add tests for coverage

* fix: Release reader from memory and typo

* fix: Reset settings that would cause other test to fail

* fix: Update authors

* fix: Replace some tests values to use more modern assertions

* fix: Change assertion

* fix: Remove creative assertions

* fix: Comment

* fix: Refactor path names usage in tests

* fix: Refactor path names usage in tests 2

* fix: Added a test case

* fix: Refactor benchmark

* fix: Target PyPi for release isntead of testPyPi

---------

Co-authored-by: tmathern <[email protected]>
Co-authored-by: Tania Mathern <[email protected]>
Co-authored-by: Alex Tran <[email protected]>
Co-authored-by: Alex <[email protected]>
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.

3 participants