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

Removed C dependency from binary exporter #395

Merged
merged 5 commits into from
Sep 11, 2024

Conversation

SebastianSchildt
Copy link
Collaborator

@SebastianSchildt SebastianSchildt commented Aug 21, 2024

This removes the C dependency from the binary, making it pure Python.

That would make it also usable when installing the Pypi packages without compiling any additional dll

@UlfBj can you check this works for you? Only changes are in vss2binary.py

@sschleemilch
Copy link
Collaborator

Looks pretty good to me, would be great to remove that dependency 👍

@SebastianSchildt SebastianSchildt marked this pull request as ready for review August 21, 2024 12:26
@erikbosch
Copy link
Collaborator

@SebastianSchildt - now there are conflicts

@SebastianSchildt SebastianSchildt force-pushed the feature/binary-pure-python branch from a5130ce to 30ac48d Compare September 2, 2024 21:31
Signed-off-by: Sebastian Schildt <[email protected]>
@SebastianSchildt
Copy link
Collaborator Author

@SebastianSchildt - now there are conflicts

Rebased. Added some additional make-up to pass our new ruff bouncer

Copy link
Collaborator

@erikbosch erikbosch left a comment

Choose a reason for hiding this comment

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

Looks good to me

@erikbosch
Copy link
Collaborator

@UlfBj - could you check if output is compatible

@erikbosch
Copy link
Collaborator

MoM:

  • Ulf has not been able to test successfully, discussed in Slack. Great if it could be checked if it works for Ted
  • Discussed in VISS slack (first)
  • Great if more persons can test
  • Sebastian: We miss quickstart for poetry (how to start vss-tools)
  • Daniel: WoW - install poetry which can install venv
  • Great if a PR to add it to a README could be created

@erikbosch
Copy link
Collaborator

I did a test on my Debian AMD64 VM comparing the output generated by this PR and latest master and the files were identical.

@UlfBj - what type of errors did you experience. Was it in generating the binary file, or that the binary file seems to be faulty? If it is latter case how can one try to reproduce it?

@erikbosch
Copy link
Collaborator

erikbosch commented Sep 10, 2024

To me it seems that the build problems that Ulf experience is unrelated to this PR itself as he experience them also for master branch. I tested with a virgin Debian VM and did not experience any build problems, and the output generated with/without this PR was identical. Suggest merge decision at the meeting tonight

@erikbosch
Copy link
Collaborator

MoM:

  • Merge

@erikbosch erikbosch merged commit 479d4ca into COVESA:master Sep 11, 2024
5 checks passed
@erikbosch erikbosch deleted the feature/binary-pure-python branch September 11, 2024 10:45
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