Merge ~alfonsosanchezbeato/network-manager:backport-wowlan-cosmic into network-manager:cosmic

Proposed by Alfonso Sanchez-Beato
Status: Merged
Approved by: Tony Espy
Approved revision: 8105af2d68d080b3d0b960490a9092888f2e9c9b
Merged at revision: 18fdc4f4648d4ec7d1a6d5d4db110fe3b1aadb75
Proposed branch: ~alfonsosanchezbeato/network-manager:backport-wowlan-cosmic
Merge into: network-manager:cosmic
Diff against target: 384 lines (+362/-0)
3 files modified
debian/changelog (+6/-0)
debian/patches/Import-some-missing-WoWLAN-patches-from-1.14.patch (+355/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Sebastien Bacher Needs Information
Tony Espy Approve
Review via email: mp+349468@code.staging.launchpad.net

Commit message

debian/patches/Support-for-WoWLAN.patch: backport WoWLAN support

Import some missing WoWLAN patches from NM upstream 1.14 (LP: #1781597)

To post a comment you must log in.
Revision history for this message
Tony Espy (awe) wrote :

I've compared this to upstream, and the only thing that seems odd is that the last four commits you list are not part of any 1.12.x branch, but only exist in master:

ac1302793 platform: add methods to retrieve current WoWLAN state
c6e40215e devices: restore past WoWLAN when disconnecting wifi
a3289400d wifi: ensure wake-on-wlan restore only acts once
2c3a14fed platform/wifi: drop *_get_wowlan()

Looking closer, the last two are a bug fix and cleanup of unused code submitted by upstream developers, whereas the first two are commits from you. Why didn't these land in 1.12?

Given these differences, your MP description and debian/changelog aren't accurate, as in addition to backporting from 1.12, you've also pulled from master too. I'm not as concerned about the MP description (although it would've been helpful in advance), but you should mention this in the changelog, and maybe in the patch too.

Can you also please update the dates in your debian/changelog and .patch file to be current?

Also have you pushed versions to a PPA anywhere that can be tested, or do you want to just wait and test the version that gets pushed to bionic-proposed?

review: Needs Fixing
Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

@Tony, better if we comment on the bionic MP only, as cosmic has 1.12 already.

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

@Alfonso

As pointed out in the other MP, not all of the patches landed in 1.12, are you going to re-work this MP, or close it out, and create a new MP for the missing patches?

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

@Tony, I will rebase to 1.12. Marking as WIP for the moment.

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

@Tony, branch updated to include patches that were not part of 1.12.

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

Thanks for the changes, looks good to me!

review: Approve
Revision history for this message
Sebastien Bacher (seb128) wrote :

Thank you for your work, some small questions

* You didn't reply to Tony about why those didn't get merged in the 1.12 branch upstream?

* 2c3a14fed doesn't seem to have an functional interest/to be only code cleaning, why do we include it in the SRU? We should limit the code changes to what is strictly needed

* why did you delete that commented patch from the series file, that's unrelevant diff for a SRU and not documented in the changelog

review: Needs Information
Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

Hi Sebastien:

* They were merged, but did not arrive on time for 1.12 and are part of 1.14. It was just a timing issue.

* I can remove that one if you want

* Right, I can put that back too

Revision history for this message
Sebastien Bacher (seb128) wrote :

Ok, not having those commit in 1.12 is fine, but yes it would be better if you could remove the unnecessary changes, thanks!

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

@Sebastien, I have refreshed now the MP.

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

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