D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

This revision was automatically updated to reflect the committed changes.
Closed by commit R282:88f3093c5237: [proxysetting] Fix build with NM 1.4 
(authored by wbauer).

REPOSITORY
  R282 NetworkManagerQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17529?vs=47446=47454

REVISION DETAIL
  https://phabricator.kde.org/D17529

AFFECTED FILES
  src/settings/proxysetting.h

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Jan Grulich]

jgrulich accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

wbauer edited the summary of this revision.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

wbauer requested review of this revision.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

wbauer updated this revision to Diff 47446.
wbauer edited the summary of this revision.
wbauer added a comment.


  Don't define the enum, rather use the numbers directly

REPOSITORY
  R282 NetworkManagerQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17529?vs=47426=47446

REVISION DETAIL
  https://phabricator.kde.org/D17529

AFFECTED FILES
  src/settings/proxysetting.h

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Jan Grulich]

jgrulich added a comment.


  In D17529#375913 , @wbauer wrote:
  
  > In D17529#375909 , @jgrulich 
wrote:
  >
  > > So maybe go with the easiest approach and just instead of defines use 0 
and 1?
  >
  >
  > Fine with me.
  >
  > Should I make that conditional depending on the NM version maybe, or just 
unconditionally use the numbers?
  
  
  Unconditionally. It doesn't matter if it uses defines or numbers directly.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

wbauer added a comment.


  In D17529#375909 , @jgrulich wrote:
  
  > So maybe go with the easiest approach and just instead of defines use 0 and 
1?
  
  
  Fine with me.
  
  Should I make that conditional depending on the NM version maybe, or just 
unconditionally use the constants?

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Jan Grulich]

jgrulich added a comment.


  So maybe go with the easiest approach and just instead of defines use 0 and 1?

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

wbauer added a comment.


  Hm, maybe I should add `extern "C"` though?
  This is inside `G_BEGIN_DECLS`/`G_END_DECLS` in nm-setting-proxy.h, which are 
defined as `extern "C" {` and `}` (according to the docs).

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

wbauer added a comment.


  In D17529#375882 , @wbauer wrote:
  
  > I don't think that really is a problem here though, as it is defined in 
libnm/nm-setting-proxy.h (in NM 1.6+) which is included by this file (via 
NetworkManager.h that's included by setting.h).
  >  I.e. the enum will still be there when building against NM 1.6.0+ 
(otherwise the build would fail in the first place anyway, as it is used just a 
few lines below).
  
  
  Sorry, I somehow read "API incompatible"...
  
  TBH, I'm not sure about ABI either. But as this is outside of any class, it 
shouldn't be a problem? (as mentioned, it would be defined by another include 
file anyway when building against  NM 1.6.0+)

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Jan Grulich]

jgrulich accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

wbauer added a comment.


  In D17529#375867 , @jgrulich wrote:
  
  > I think the easiest solution here is just to set 0 and 1 to the enum 
values, instead of NM defines.
  
  
  Sure, that would work as well.
  
  But IMHO the version check would make it easier to remove this "hack" when 
the minimum NM version would be raised at some point in the future.
  
  Whatever you prefer though.
  
  > I'm not sure if adding/removing enum is ABI compatible change, because the 
enum will disapper once you build it against NM 1.6.0+.
  
  Hm, I see what you mean.
  
  I don't think that really is a problem here though, as it is defined in 
libnm/nm-setting-proxy.h (in NM 1.6+) which is included by this file (via 
NetworkManager.h that's included by setting.h).
  I.e. the enum will still be there when building against NM 1.6.0+ (otherwise 
the build would fail in the first place anyway, as it is used just a few lines 
below).

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Jan Grulich]

jgrulich added a comment.


  I think the easiest solution here is just to set 0 and 1 to the enum values, 
instead of NM defines. I'm not sure if adding/removing enum is ABI compatible 
change, because the enum will disapper once you build it against NM 1.6.0+.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

wbauer added a reviewer: jgrulich.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

To: wbauer, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17529: [proxysetting] Fix build with NM 1.4

[Wolfgang Bauer]

wbauer created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
wbauer requested review of this revision.

REVISION SUMMARY
  The `NMSettingProxyMethod` enum only exists since NetworkManager 1.6.
  So define it appropriately when building with a lower version (i.e. 1.4.x).

TEST PLAN
  Builds fine now with NM 1.4.x, before there were compiler errors:
  
In file included from 
/home/abuild/rpmbuild/BUILD/networkmanager-qt-VERSIONgit.20181211T113515~4332597/src/settings/proxysetting.cpp:21:0:

/home/abuild/rpmbuild/BUILD/networkmanager-qt-VERSIONgit.20181211T113515~4332597/src/settings/proxysetting.h:43:16:
 error: 'NM_SETTING_PROXY_METHOD_NONE' was not declared in this scope
 None = NM_SETTING_PROXY_METHOD_NONE,
^

/home/abuild/rpmbuild/BUILD/networkmanager-qt-VERSIONgit.20181211T113515~4332597/src/settings/proxysetting.h:44:16:
 error: 'NM_SETTING_PROXY_METHOD_AUTO' was not declared in this scope
 Auto = NM_SETTING_PROXY_METHOD_AUTO
^
  
  Still compiles with newer versions, tested with 1.6.0, 1.10.6, and 1.14.4.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17529

AFFECTED FILES
  src/settings/proxysetting.h

To: wbauer
Cc: kde-frameworks-devel, michaelh, ngraham, bruns