jimbraun / XCDF

XCDF: eXplicitly Compacted Data Format. See documentation at Read the Docs:
https://xcdf.readthedocs.io/en/latest/
Other
14 stars 8 forks source link

New python bindings using pybind11 #93

Closed maxnoe closed 1 year ago

maxnoe commented 2 years ago

Here I replace the existing python bindings which are not working with python3 with new bindings using the excellent pybind11 library.

TODO (for @HealthyPear):

I for now removed the histogram class, which was pure python 2 and seemed unrelated to the xcdf IO functionality, let me know what you think.

Also if you think that anything in the API is missing.

HealthyPear commented 2 years ago

Hi @jimbraun,

I added a CMake-based continuous integration pipeline for the C++ integration with python bindings (working on the pure-python one).

You should be able to activate it and make it a requirement for merging from settings-->branches-->Branch protection rules (for the masterbranch) --> Require status checks to pass before merging--> Status checks

HealthyPear commented 2 years ago

Dear @jimbraun ,

I added also a CI based entirely on Python, so you can add also that to the required status checks for merging. Unfortunately we have a problem in the macos installation (ubuntu works fine) so while the CMake-based pipeline will succeed the Python one will fail for the moment.

Next steps:

HealthyPear commented 2 years ago

Dear @jimbraun ,

I now added a (simple) Python testing function to make sure we correctly read an XCDF file written by the C++ interface. The macOS installation issue should be fixed, but I could test it only manually.

It seems a good time to enable the CI pipelines (neither me or my colleague can, since we are not administrators): settings-->branches-->Branch protection rules (for master only is sufficient) --> Require status checks to pass before merging--> Status checks.

The CI pipelines are 2:

There are still few things missing like, e.g.

jimbraun commented 2 years ago

@HealthyPear : Thanks much for adding the CI pipelines. I agree and I'll enable these next week.

jimbraun commented 1 year ago

I merged the CI pipelines manually into master, and checks are now required to pass before merging. Note that the Python checks currently fail in master, but merging this PR should be the next action in XCDF, so I feel this is OK for now.

HealthyPear commented 1 year ago

For what concerns early development of SWGO software (my use case) I don't think I need to export any more API for the time being. With successful checks, I am satisfied with this contribution unless you (@jimbraun and/or @maxnoe) spot some mistakes.

jimbraun commented 1 year ago

I'm satisfied with these changes and happy to merge the PR when the few remaining issues above have been resolved.

HealthyPear commented 1 year ago

@jimbraun I don't know why, but now it seems I need you to approve the workflows (they were running with not problem before...) - see here (I hope it's a one-time operation)

maxnoe commented 1 year ago

@HealthyPear I guess it's because I pushed changes and I wasn't approved yet

jimbraun commented 1 year ago

@HealthyPear : I've been manually approving all the workflow runs. They don't start automatically for first-time contributors.

HealthyPear commented 1 year ago

@jimbraun could please trigger the workflows? @maxnoe do you see something else that needs to be fixed in this PR?

HealthyPear commented 1 year ago

@jimbraun could you do it again? I think we are done @maxnoe?

For the moment I think this PR has what needs to read XCDF data to Python3 in a basic way, aside from Python packages like astropy.

HealthyPear commented 1 year ago

@maxnoe if everything is ready I think you have to change the status of the PR from Draft

maxnoe commented 1 year ago

From my side good!

HealthyPear commented 1 year ago

I am also satisfied!

jimbraun commented 1 year ago

Thanks @HealthyPear and @maxnoe!