-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat(snap): Add snap packaging #30
Conversation
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my inline comments, there are quite a few open questions which need answers.
snap/snapcraft.yaml
Outdated
@@ -0,0 +1,134 @@ | |||
name: edgex-device-usb-camera | |||
base: core20 | |||
license: Apache-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the camera service itself is licensed as Apache 2.0, some of the other components in this snap have other licenses (e.g. rtsp-simple-server is MIT).
Also this snap is including ffmpeg which is licensed under LGPLv2, however some components of ffmpeg are licensed under GPLv2 (see https://www.ffmpeg.org/legal.html). Was this reviewed as part of the new service review?
Same question about libpulse0 (which again is licensed under LGPLv2)? The pulseaudio license also mentions that if D-Bus support is enabled, the PulseAudio client library (libpulse) MAY need to be licensed under the GPL, depending on the license adopted for libdbus. Also do we know why service requires libpulse0? Is there a runtime requirement for the PulseAudio server in order to use this service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we know why service requires libpulse0? Is there a runtime requirement for the PulseAudio server in order to use this service?
It is needed by pprobe
. Added a comment.
Edit: turns out that libpulseaudio it was already there, just that it was missing from path. Removed it as staged package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the camera service itself is licensed as Apache 2.0, some of the other components in this snap have other licenses (e.g. rtsp-simple-server is MIT).
This should be fine since MIT is permissive.
I've also considered not including rtsp server in the snap and let users install their own server (e.g. v4l2-rtspserver). But in the end, I decided to keep it since it is only 9MB and disabled by default. The docker image includes it as well.
Also this snap is including ffmpeg which is licensed under LGPLv2, however some components of ffmpeg are licensed under GPLv2 (see https://www.ffmpeg.org/legal.html). Was this reviewed as part of the new service review?
I did not review the docker base images and packages, not sure if someone else did. @iain-anderson @lenny-intel an idea?
This would be a project issue and not specific to the snap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to explicit section in GNU's license agreement (section 5 in the document linked below), we are ok to use the library and still distribute as Apache 2. https://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. This is known as the linking exception. The same was applied when we worked with 0MQ. LF legal cannot provide legal advise (strange but true), but their reading of this also pointed us to this exception. (see https://en.wikipedia.org/wiki/GPL_linking_exception).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpwhitemn you're misinterpreting my comment. I acknowledged that the ffmpeg package is licensed as LGPLv2, however there are optional parts of ffmpeg that are licensed as GPLv2. My question was simply did we review our usage of ffmpeg to ensure we didn't use one of these optional parts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell the ffmpeg support doesn't link the ffmpeg library, it runs an ffmpeg command as a separate process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that the snap metadata is saying the license of this snap is "Apache 2.0", and that's not technically correct, as the snap (and most likely our docker image) contains more than just the device service Go binary. The guidance from the snapcraft.io reference for snapcraft.yaml
say this needs to be an SPDX license expression (see https://spdx.dev/spdx-specification-21-web-version/), which in this case should be a list of licenses as there's doesn't seem to be an option for "multiple licenses" anymore.
For instance, here are a number of shared libraries that and up in the snap and their associated licenses:
- libpango-1.0.0 - LGPL2
- libvorbisenc2 - BSD
- libxvidcore4 - GPL2+
And also while reviewing the files in the snap, I don't see a license file for the rtsp-server itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are close to 200 packages pulled in for ffmpeg and libzmq5. Many of those (incl. libzmq ones) have multiple licenses. It is simply not practical to identify and include all these, and maintain them over time. I don't see any other Docker image or snap listing all licenses.
I found some useful information at:
- https://www.linuxfoundation.org/tools/docker-containers-what-are-the-open-source-licensing-considerations/
- https://opensource.stackexchange.com/a/8393
I believe the license in the Dockerfile is the Dockerfile's license, rather than the license of Docker images built with it. For the snap, the license field appear to be the license of the resulting snap packages. So, IMO, it is most appropriate to remove the optional license
field. All licenses remain available under snap's /usr/share/doc
directory.
The license for rtsp-simple-server is at snap's /usr/share/doc/rtsp-simple-server/LICENSE
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the license field from snapcraft.yaml: 4c4ab6a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farshidtz Thanks a lot for the PR. I have tested it with secure and non-secure modes. Both work as expected for video streaming with my camera Aukey-PC-LM1E.
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
This reverts commit 36ec5bb. Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
- remove sideloaded Ubuntu 22.04 videodev header - add shared lib paths - remove libpulse0 as it already exists Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
It builds fine with snapcraft 6 (current latest/stable) |
We can now build the snap using snapcraft 7.0.4 but there are some unresolved issues in regards to packaging of the hooks and licenses. |
This is a workaround for a Snapcraft 7 bug: https://bugs.launchpad.net/snapcraft/+bug/1980089 Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Snapcraft 7.0.8 packages the licenses but the hooks issue isn't resolved yet. I have reported it here. In the meantime, I've added a workaround (4eccf07) which solves the issue for this snap. Another thing I noticed when building was an added 15MB to the size of the snap. It looks like many additional files are being packaged automatically from dependencies (e.g. under usr/share apart from docs). We'll have to dig deeper and remove them in future PRs. |
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farshidtz Thanks for the hook refactor.
I have tested this snap. .env files and config files are in place. Everything works as expected.
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Needed when building with Snapcraft 7, perhaps as a result of different part sourcing workflow. This is also a better approach because we won't add files to source files (outside the snap) during the build. Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
Found and fixed a bug while working on the tests. The tests don't run on this PR yet because Github Actions get enabled for the first time after this PR has merged. A sample run can be reviewed from my fork: https://github.com/farshidtz/edgex-device-usb-camera/runs/7136890125 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I able to build with snapcraft 6 natively on a 22.04 machine, things still fail for me if I try to build natively using snapcraft 7.0.8 on the same machine. If you want the details, I can provide.
I was able to successfully install the snap, however the testing instructions weren't sufficient for me to actually test the snap.
- any suggestions on what camera I should use?
- what are the changes I need to make to the device yaml file?
Also note, there are still some issues with licensing (please see inline comment).
``` | ||
snapcraft -v | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should mention that building this snap remains "challenging" and at least offer some options. I don't think the given option "just works".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained in another comment, we have been able to build it without problems locally and on CI servers. If there are any OS-specific challenges related to using snapcraft to build core22-based snaps, it should be raised on snapcraft issue trackers.
Please refer [here][secret-store-token] for further information. | ||
|
||
### Connect the camera interface | ||
The [`camera`](https://snapcraft.io/docs/camera-interface) interface is currently **NOT automatically connected** as it is pending auto-connection permissions from the store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we requested these permissions yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. We will do so once we have a stable source and build that can be reviewed by the store.
@@ -0,0 +1,3 @@ | |||
#!/bin/bash | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a workaround for snapcraft 7, correct? Did you ever get a response from the snapcraft team on this? At minimum, we should probably add a note to this script noting that it's a workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in this comment, I've reported the bug.
We are no longer affected by it because we have refactored the hooks to have a single binary executable. This reduces the build complexity (code, time) and snap size. The same improvement is being rolled out to other snaps.
snap/snapcraft.yaml
Outdated
@@ -0,0 +1,134 @@ | |||
name: edgex-device-usb-camera | |||
base: core20 | |||
license: Apache-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that the snap metadata is saying the license of this snap is "Apache 2.0", and that's not technically correct, as the snap (and most likely our docker image) contains more than just the device service Go binary. The guidance from the snapcraft.io reference for snapcraft.yaml
say this needs to be an SPDX license expression (see https://spdx.dev/spdx-specification-21-web-version/), which in this case should be a list of licenses as there's doesn't seem to be an option for "multiple licenses" anymore.
For instance, here are a number of shared libraries that and up in the snap and their associated licenses:
- libpango-1.0.0 - LGPL2
- libvorbisenc2 - BSD
- libxvidcore4 - GPL2+
And also while reviewing the files in the snap, I don't see a license file for the rtsp-server itself.
@tonyespy Thanks for the review.
The snap build and tests (added in this PR) pass on Github (https://github.com/farshidtz/edgex-device-usb-camera/runs/7136890125; see comments above) and Launchpad (https://launchpad.net/~canonical-edgex/+snap/draft-edgex-device-usb-camera). Also builds fine on my machine (Ubuntu 22.04) and @MonicaisHer's. I'm not sure what issue you have faced. Maybe a clean build will resolve it?
This PR is just adding snap packaging. Please refer to the README: https://github.com/edgexfoundry/device-usb-camera#tested-devices Changes may be required to the local device path if you have more than one connected USB camera. |
License files are packaged under /usr/share/doc Signed-off-by: Farshid Tavakolizadeh <farshid.tavakolizadeh@canonical.com>
It definitely failed for me using snapcraft 7.0.8 on Jammy, but it's now working with 7.0.9, so not worth investigating any further.
I was reacting to the fact that you provided some instructions, but then in the middle left out a bunch of details. I'm reviewing a PR not the service. While not required, detailed test instructions make the process of reviewing easier for the reviewers. So that said, I'm not going to test anything more than installing the snap.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on our conversation earlier today, I'm approving this PR "as is".
Thanks for the reviews. This PR is ready to merge, leaving the following as future work:
@cloudxxx8 could you please squash and merge? Please remove all the commits history from squash commit description. |
PR Checklist
Please check if your PR fulfills the following requirements:
If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-examples/blob/master/.github/CONTRIBUTING.md.
What is the current behavior?
Issue Number:
What is the new behavior?
Does this PR introduce a breaking change?
New Imports
Are there any new imports or modules? If so, what are they used for and why?
Specific Instructions
Building locally is possible using Snapcraft 7:
You can skip this and install the pre-built snap for this PR.
Configure and start:
Start streaming:
Other information
The snap is 130MB+. It may be possible to reduce the size by removing unused shared library object files pulled in as ffmpeg dependencies.