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

Logitech bulk controller plugin initial commit #3609

Merged
merged 2 commits into from
Aug 29, 2021
Merged

Conversation

vcdmp
Copy link
Collaborator

@vcdmp vcdmp commented Aug 6, 2021

Type of pull request:

  • [ x] New plugin (Please include new plugin checklist)
  • Code fix
  • Feature
  • Documentation
  • Fill out README.md with update protocol
  • Fill out README.md with any custom quirks and flags
  • Fill out README.md with the vendor ID security value
  • Implement FuFirmware->write() and include at least one fuzzer testcase in src/fuzzing/firmware for any custom FuFirmware subclass
  • CI run of the plugin for at least one target
  • Document targets CI isn't currently run and the reasons (i.e. minimum versions needed or distribution limitations etc).

@vcdmp vcdmp requested a review from hughsie August 6, 2021 08:31
meson_options.txt Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
@superm1
Copy link
Member

superm1 commented Aug 6, 2021

@djcampello: I think we need to add some extra stuff to clang format for protobuf:

Configuration file(s) do(es) not support Proto: /home/runner/work/fwupd/fwupd/.clang-format

@superm1
Copy link
Member

superm1 commented Aug 6, 2021

Can you please add to dependencies.xml some development packages for protobuf in the various distros so that CI can run? Get at least one that you're familiar with done and we can help fill in the others if you can't find the package names for it.

@superm1

This comment has been minimized.

contrib/ci/dependencies.xml Outdated Show resolved Hide resolved
@superm1 superm1 force-pushed the wip/bulkcontroller branch 2 times, most recently from 83437b3 to 4441eb9 Compare August 7, 2021 02:19
@superm1
Copy link
Member

superm1 commented Aug 7, 2021

Since master just had an ABI break for the plugin, I rebased on master and adjusted your code for that ABI break.
I also sorted out the dependencies needed for all the distros, so all of the CI should hopefully be building now, and hopefully I didn't break anything you had working.

Beyond a bunch of nits like function names and where to have prototypes and stuff - I think you have some error handling you need to adjust, and also some changed to store device data to subclassed devices instead of the plugin.

@superm1
Copy link
Member

superm1 commented Aug 7, 2021

@djcampello: I think we need to add some extra stuff to clang format for protobuf:

Configuration file(s) do(es) not support Proto: /home/runner/work/fwupd/fwupd/.clang-format

@djcampello If you can please review what I did to clang-format to make protobuf stuff work, would appreciate it.

@superm1
Copy link
Member

superm1 commented Aug 9, 2021

I tried and left some comments in the build script for our future selves on what it would take (technically) to enable Windows.
It's mostly blocked by https://bugzilla.redhat.com/show_bug.cgi?id=1991749.
If that's fixed some day, I would like to turn it on for CI at least, even if we exclude it from the EXE on tagged releases.

@djcampello
Copy link
Collaborator

@djcampello: I think we need to add some extra stuff to clang format for protobuf:

Configuration file(s) do(es) not support Proto: /home/runner/work/fwupd/fwupd/.clang-format

@djcampello If you can please review what I did to clang-format to make protobuf stuff work, would appreciate it.

This looks good to me. I am not completely familiar with per-language configuration but it looks good!

superm1
superm1 previously approved these changes Aug 10, 2021
Copy link
Member

@superm1 superm1 left a comment

Choose a reason for hiding this comment

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

I have two very small open inquiries about this, but otherwise it looks good to me. Feel free to squash/force push my recent changes if you agree with them.

@superm1
Copy link
Member

superm1 commented Aug 17, 2021

I have two very small open inquiries about this, but otherwise it looks good to me. Feel free to squash/force push my recent changes if you agree with them.

@vcdmp can you check those two nits and comment? I think we're ready to merge once you've answered those and tested the branch again.

@superm1 superm1 force-pushed the wip/bulkcontroller branch 2 times, most recently from 0f8f041 to 21a8e7a Compare August 25, 2021 01:28
@vcdmp vcdmp requested a review from superm1 August 27, 2021 18:44
@superm1
Copy link
Member

superm1 commented Aug 27, 2021

LGTM, can you please squash into one commit?

@superm1
Copy link
Member

superm1 commented Aug 27, 2021

LGTM, can you please squash into one commit?

squash your two I mean - leave the protobuf clang one separate

@vcdmp vcdmp requested a review from hughsie August 28, 2021 19:24
@vcdmp vcdmp requested a review from hughsie August 29, 2021 18:41
@hughsie hughsie merged commit b6ff1ea into master Aug 29, 2021
@delete-merged-branch delete-merged-branch bot deleted the wip/bulkcontroller branch August 29, 2021 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants