cmd/snap-gpio-helper: add skeleton with re-exec support #15023

ZeyadYasser · 2025-02-04T14:19:30Z

No description provided.

github-actions · 2025-02-04T14:42:38Z

Thu Feb 6 11:43:42 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13176027131

Failures:

Executing:

google-core:ubuntu-core-24-64:tests/core/gadget-update-pc

Restoring:

google-core:ubuntu-core-24-64:tests/core/gadget-update-pc
google-core:ubuntu-core-24-64:tests/core/
google-core:ubuntu-core-24-64

codecov · 2025-02-04T15:11:46Z

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@ff29f60). Learn more about missing BASE report.
Report is 75 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/snap-gpio-helper/main.go	0.00%	15 Missing ⚠️
cmd/snap-gpio-helper/cmd_export.go	0.00%	2 Missing ⚠️
cmd/snap-gpio-helper/cmd_unexport.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #15023   +/-   ##
=========================================
  Coverage          ?   78.21%           
=========================================
  Files             ?     1168           
  Lines             ?   154412           
  Branches          ?        0           
=========================================
  Hits              ?   120779           
  Misses            ?    26192           
  Partials          ?     7441

Flag	Coverage Δ
unittests	`78.21% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Zeyad Gouda <[email protected]>

bboozzoo · 2025-02-05T14:01:48Z

cmd/snap-gpio-helper/main.go

+	"github.com/jessevdk/go-flags"
+	"github.com/snapcore/snapd/snapdtool"


Suggested change

"github.com/jessevdk/go-flags"

"github.com/snapcore/snapd/snapdtool"

"github.com/jessevdk/go-flags"

"github.com/snapcore/snapd/snapdtool"

bboozzoo · 2025-02-05T14:02:03Z

packaging/ubuntu-14.04/snapd.install

@@ -24,6 +24,7 @@ usr/bin/snapd /usr/lib/snapd/
 usr/lib/snapd/snap-confine
 usr/lib/snapd/snap-device-helper
 usr/lib/snapd/snap-discard-ns
+usr/lib/snapd/snap-gpio-helper


I'd drop it here, maybe just leave a comment

Signed-off-by: Zeyad Gouda <[email protected]>

Meulengracht · 2025-02-06T08:28:24Z

cmd/snap-gpio-helper/cmd_unexport.go

+
+import "errors"
+
+type cmdUnexportChardev struct {


Does the spec not mention "<chip-labels>" "<line>" "<gadget>" "<slot-name>" for this command?

for unexporting they are not really needed, but I could add them for completeness.

Actually the description for chip-labels and lines wouldn't make sense in the case of unexporting.

ChipLabels string `long:"chip-labels" description:"comma-separated list of source chip label(s) to match" required:"yes"` Lines uint `long:"line" description:"comma-separated list of gpio line(s) to unexport" required:"yes"`

options are marked as required, so you could make them positional like for export. Also, let's add all of them, such that even if the implementation was to change we have enough context provided in the input.

Meulengracht · 2025-02-06T08:28:46Z

cmd/snap-gpio-helper/cmd_export.go

+)
+
+type cmdExportChardev struct {
+	ChipLabels []string `long:"chip-label" description:"source chip label(s) to match" required:"yes"`


The spec mentions plurals of these:

Suggested change

ChipLabels []string `long:"chip-label" description:"source chip label(s) to match" required:"yes"`

ChipLabels []string `long:"chip-labels" description:"source chip label(s) to match" required:"yes"`

I thought it would be more readable to pass multiple --chip-label args instead of one large comma-separated entry --chip-label label-1 --chip-label label-2 vs --chip-labels label-1,label-2.

After second thought, the latter looks more compact.

Meulengracht · 2025-02-06T08:28:53Z

cmd/snap-gpio-helper/cmd_export.go

+
+type cmdExportChardev struct {
+	ChipLabels []string `long:"chip-label" description:"source chip label(s) to match" required:"yes"`
+	Lines      []uint   `long:"line" description:"gpio line(s) to export" required:"yes"`


Suggested change

Lines []uint `long:"line" description:"gpio line(s) to export" required:"yes"`

Lines []uint `long:"lines" description:"gpio line(s) to export" required:"yes"`

…-separated Signed-off-by: Zeyad Gouda <[email protected]>

bboozzoo · 2025-02-06T09:02:59Z

cmd/snap-gpio-helper/cmd_export.go

+	ChipLabels string `long:"chip-labels" description:"comma-separated list of source chip label(s) to match" required:"yes"`
+	Lines      uint   `long:"line" description:"comma-separated list of gpio line(s) to export" required:"yes"`
+	Gadget     string `long:"gadget" description:"gadget snap name" required:"yes"`
+	Slot       string `long:"slot" description:"gpio-chardev slot name" required:"yes"`


all are required so you can make them positional

bboozzoo · 2025-02-06T09:03:12Z

cmd/snap-gpio-helper/cmd_export.go

+
+type cmdExportChardev struct {
+	ChipLabels string `long:"chip-labels" description:"comma-separated list of source chip label(s) to match" required:"yes"`
+	Lines      uint   `long:"line" description:"comma-separated list of gpio line(s) to export" required:"yes"`


Suggested change

Lines uint `long:"line" description:"comma-separated list of gpio line(s) to export" required:"yes"`

Lines string `long:"line" description:"comma-separated list of gpio line(s) to export" required:"yes"`

my bad, thanks!

bboozzoo · 2025-02-06T09:07:42Z

cmd/snap-gpio-helper/cmd_unexport.go

+
+import "errors"
+
+type cmdUnexportChardev struct {


options are marked as required, so you could make them positional like for export. Also, let's add all of them, such that even if the implementation was to change we have enough context provided in the input.

Signed-off-by: Zeyad Gouda <[email protected]>

bboozzoo

I think Debian packaging will ship gpio-helper by default. We probably need to remove it explicitly

Meulengracht · 2025-02-07T11:48:46Z

I think Debian packaging will ship gpio-helper by default. We probably need to remove it explicitly

right this needs to be checked

ZeyadYasser force-pushed the gpiod-helper-skeleton branch from aef6aa8 to 38c2d57 Compare February 4, 2025 14:53

ZeyadYasser marked this pull request as ready for review February 4, 2025 15:58

ZeyadYasser closed this Feb 4, 2025

ZeyadYasser reopened this Feb 4, 2025

ZeyadYasser force-pushed the gpiod-helper-skeleton branch from 38c2d57 to 045943a Compare February 5, 2025 09:25

ZeyadYasser added 3 commits February 5, 2025 15:43

cmd/snap-gpio-helper: add skeleton with re-exec support

44d70df

Signed-off-by: Zeyad Gouda <[email protected]>

packaging: include snap-gpio-helper command binary for ubuntu

2566835

Signed-off-by: Zeyad Gouda <[email protected]>

tests: add basic test to check snap-gpio-helper binary is installed

a74f848

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser force-pushed the gpiod-helper-skeleton branch from 045943a to a74f848 Compare February 5, 2025 13:47

ZeyadYasser requested review from bboozzoo and Meulengracht February 5, 2025 13:48

bboozzoo reviewed Feb 5, 2025

View reviewed changes

ZeyadYasser added 2 commits February 5, 2025 16:14

fixup! cmd/snap-gpio-helper: improve formatting

51d0998

Signed-off-by: Zeyad Gouda <[email protected]>

fixup! packaging: drop snap-gpio-helper from ubuntu-14.04

995a559

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser requested a review from bboozzoo February 5, 2025 14:18

Meulengracht reviewed Feb 6, 2025

View reviewed changes

fixup! cmd/snap-gpio-helper: switch chip labels and lines to be comma…

1f5f3b2

…-separated Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser requested a review from Meulengracht February 6, 2025 09:01

bboozzoo reviewed Feb 6, 2025

View reviewed changes

fixup! cmd/snap-gpio-helper: switch to positional arguments

a82d3d4

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser requested a review from bboozzoo February 6, 2025 09:46

bboozzoo reviewed Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/snap-gpio-helper: add skeleton with re-exec support #15023

cmd/snap-gpio-helper: add skeleton with re-exec support #15023

ZeyadYasser commented Feb 4, 2025

github-actions bot commented Feb 4, 2025 •

edited

Loading

codecov bot commented Feb 4, 2025 •

edited

Loading

bboozzoo Feb 5, 2025

bboozzoo Feb 5, 2025

Meulengracht Feb 6, 2025

ZeyadYasser Feb 6, 2025

ZeyadYasser Feb 6, 2025

bboozzoo Feb 6, 2025

Meulengracht Feb 6, 2025

ZeyadYasser Feb 6, 2025

Meulengracht Feb 6, 2025

bboozzoo Feb 6, 2025

bboozzoo Feb 6, 2025

ZeyadYasser Feb 6, 2025

bboozzoo Feb 6, 2025

bboozzoo left a comment

Meulengracht commented Feb 7, 2025

		"github.com/jessevdk/go-flags"
		"github.com/snapcore/snapd/snapdtool"

	ChipLabels []string `long:"chip-label" description:"source chip label(s) to match" required:"yes"`
	ChipLabels []string `long:"chip-labels" description:"source chip label(s) to match" required:"yes"`

	Lines []uint `long:"line" description:"gpio line(s) to export" required:"yes"`
	Lines []uint `long:"lines" description:"gpio line(s) to export" required:"yes"`

	Lines uint `long:"line" description:"comma-separated list of gpio line(s) to export" required:"yes"`
	Lines string `long:"line" description:"comma-separated list of gpio line(s) to export" required:"yes"`

cmd/snap-gpio-helper: add skeleton with re-exec support #15023

Are you sure you want to change the base?

cmd/snap-gpio-helper: add skeleton with re-exec support #15023

Conversation

ZeyadYasser commented Feb 4, 2025

github-actions bot commented Feb 4, 2025 • edited Loading

Failures:

Executing:

Restoring:

codecov bot commented Feb 4, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

Meulengracht commented Feb 7, 2025

github-actions bot commented Feb 4, 2025 •

edited

Loading

codecov bot commented Feb 4, 2025 •

edited

Loading