Merge lp://staging/~morphis/network-manager/add-snappy-support into lp://staging/~phablet-team/network-manager/snappy

Proposed by Simon Fels
Status: Merged
Approved by: Tony Espy
Approved revision: 978
Merged at revision: 972
Proposed branch: lp://staging/~morphis/network-manager/add-snappy-support
Merge into: lp://staging/~phablet-team/network-manager/snappy
Diff against target: 4420 lines (+4353/-0)
12 files modified
debian/patches/add-snappy-support.patch (+691/-0)
debian/patches/fix-code-to-build-with-werror.patch (+547/-0)
debian/patches/series (+3/-0)
parts/plugins/x-autotools.py (+129/-0)
snapcraft.yaml (+239/-0)
snappy/bin/dhclient (+32/-0)
snappy/bin/dnsmasq (+41/-0)
snappy/bin/networkmanager (+37/-0)
snappy/conf/NetworkManager.conf (+6/-0)
snappy/conf/dnsmasq-dbus.conf (+10/-0)
snappy/conf/org.freedesktop.NetworkManager.conf (+152/-0)
snappy/icon.svg (+2466/-0)
To merge this branch: bzr merge lp://staging/~morphis/network-manager/add-snappy-support
Reviewer Review Type Date Requested Status
Tony Espy Approve
Simon Fels Pending
Review via email: mp+289514@code.staging.launchpad.net

Commit message

Import snappy support from our 15.04 work

This is fully converted to snapcraft 2.x syntax now but still runs fully
unconfined. Also this doesn't include ModemManager as for the snappy 16
series we still have to find out whether we go with it or ofono as default.

Additionaly this is putting the snappy build configuration next to our
debian to both together and build stuff from the same base (patches,
config, ...).

Description of the change

Still some things we need to debate like

- version number (took the nm version + -<n>)
- Applying all debian/patches/ or not

but is fine for a first drop and we can move on from what we have then in that branch by defining further work items.

To post a comment you must log in.
976. By Simon Fels

Remove left over ModemManager udev rules

Revision history for this message
Tony Espy (awe) wrote :

1) I will have to say, I'm not a fan of merging the debian and snappy packaging into a single branch. There's simply too many debian-specific files included which are useless when it comes to building a snap. Add in the fact that the debian packaging branch for NM includes many stale patches ( ie. patches that aren't used or are commented out in the d/p/series ), and it seems like a mess to me.

Also, if we base the snap on the NM branch like this, we probably should revise the version to actually reflect this, at the moment it doesn't include the '-4ubuntu15.1.8' part in the version, yet it does include the code from that version.

I really prefer the bluez approach, where although the bzr namespace is the same as the debian package, the snap branch *only* includes snappy bits.

2) Why bother with --configure-snappy, the branch is solely for snappy? If we eventually try to upstream the patch(es), we'll probably need to change this is done ( ie. doubtful upstream would accept such a switch, we'd probably instead want to fix generically ), so let's either make the branch snappy only, or re-work the snappy patch into something we *think* upstream might accept ( note, let's do this after we switch to 1.2 ).

3) Why did you remove modemmanager and leave ofono in place? We know that modemmanager works, and we also know that ofono won't work till we re-write the settings plugin.

4) What's the reasoning behind the fix-code-to-build-with-werror.patch? It seems like this is changing a lot of code. Also this may not work with NM1.2, as they've gotten rid of some deprecated code ( eg. dbus-glib ), but not all ( eg. GSimpleAsyncResult ). Does the snap fail to build without this?

Note, I've also cleaned up a lot of ofono unused code in my 1.2 branch.

5) Where did the icon.svg come from? It's *huge* compared to the bluez icon.png ( which is a generic Ubuntu icon ).

6) What's the plan to fix "XXX: hardcode architecture" in wrapper binaries?

7) It looks like you're manually installing the binary wrapper scripts. Have you tested this? I couldn't get bluez to work this way, and ended up modifying yet another core snapcraft library to add the copies in when generating the wrapper scripts.

8) I don't see nmcli?

9) d/p/0002-Force-online-state-with-unmanaged-devices.patch: not re-patched for snappy.

10) 0003-Don-t-setup-Sleep-Monitor-if-not-booted-with-systemd.patch: not applicable for snappy.

11) d/p/0004-Use-symlinks-for-nmtui.patch: have you tested nmtui on snappy?

12) There are bunch of other patches that aren't really applicable to snappy ( eg. whoopsie support, upstart patches, ... ).

review: Needs Information
Revision history for this message
Tony Espy (awe) wrote :

Also note that the bluez snap uses the ~bluetooth team, perhaps we should use ~network-manager here instead of ~phablet-team?

Revision history for this message
Simon Fels (morphis) wrote :
Download full text (4.5 KiB)

Thanks a lot for the first review round!

For 1: I must say I don't really like us separating stuff into multiple different approaches per product. We already have separate packaging branches for desktop and touch and adding a third one for snappy doesn't really scale. That there are debian patches which might not apply to snappy, stale patches etc. but those things are all sth we have to solve over time. We have to find one common approach for all projects we're doing and apply that to all projects the same way.

Also respect that we're already shipping snappy packages for 15.04 with the exact same approach we're doing here but just having the debian package unpacked in another git repo. So this is only a continuation of what we've done before but in a more packed way. I would really like to bring all the different work we're doing as close as possible together rather than investing a huge amount of time for development, testing, reviewing for all different projects. There will be always project specific development/testing/.. but we should aim for convergence. I know there are problems in practice but those are the bits we need to solve.

I am fine with adding the debian packaging version number.

For 2: Adding --enable-snappy was done with the aim to align our packaging as described for 1). The debian package would simply build with --disable-snappy.

For 3: I removed ModemManager as

 * It needs to be in its own snap. The NetworkManager snap/part
   then just interacts with. What we did for 15.04 was a
   quick hack and is nothing we should just continue to do
   without discussing what we really want.
 * Its a per device decision if a telephony snap should be
   installed or not.
 * There are different components we can use to bring
   telephony into the game and we still haven't decided
   which horse we will ride for snappy (ofono, ModemManager)
 * This MP isn't aiming for feature completeness. Its just
   bringing the basic work from 15.04 to series 16 and
   leaving stuff out we don't know if we want it or not.

Also I left ofono in place as I simply copied the configflags from the debian packaging which have ofono enabled by default. That is again sth which needs to be reworked in follow up stories after we have decided where our way goes to.

For 4: I added that patch simply because all the wonderful patches we did on top of upstream doesn't build with -Werror (deprecated glib functions, ignoring const, unused variables) which is the default on xenial. I am sure you fixed some or most of them already with your 1.2 work and lets not bother with this patch too much. Its just there to get stuff building for the time being but the right way is to rework all those patches.

For 5: That icon is what we had in 15.04 for the network-manager snap. Its a more colorful networking one coming from the default icon set on desktop??

For 6: None yet. But the best would be if we get rid of them by telling the build not to install things into arch specific directories as that doesn't make much sense with snappy. Its currently a bit of a mess how those things are handled within the snap configuration but that is up for later.

For 7: Works as it s...

Read more...

977. By Simon Fels

Move snappy icon to its folder

978. By Simon Fels

Reflect debian version also in our snap version

Revision history for this message
Tony Espy (awe) :
review: Approve
Revision history for this message
Tony Espy (awe) wrote :

Approved; discussion on most of the above points will happen separately.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches