Created attachment 218742 [details] [review]
Current OpenBSD patch

(In reply to comment #0)
> Any ideas/thoughts on how to implement this optional functionality correctly?

Add an appropriate property to js/misc/config.js and use it to make the imports conditional.

I'll help pitch in myself on portability for GTK+ and below, because honestly we need portability to Windows for strategic reasons, and if we do that, also handling random Unix variations is a rounding error.

Portability for the full system is a much larger task and consequently more controversial.  Many people in GNOME have gone back and forth on what we should do on this topic:

http://www.mail-archive.com/desktop-devel-list@gnome.org/msg16654.html

I myself have argued that we should always have GNOME be able to run in "thin client" mode.  That argument means that while a patch to make NM optional is probably not too difficult to manage, having different pluggable network stacks is an entirely different world.
 
Anyways, on your actual patch - this is why I hate that people build from tarballs.  Patches to the Makefile.in are useless.  Make a patch on top of git (https://live.gnome.org/GnomeLove/SubmittingPatches) please.

(In reply to comment #3)
[...]

> Anyways, on your actual patch - this is why I hate that people build from
> tarballs.  Patches to the Makefile.in are useless.  Make a patch on top of git
> (https://live.gnome.org/GnomeLove/SubmittingPatches) please.
As I mentioned in my original post, this patch is not intended get committed, hence the patch configure and Makefile.in scripts. When I get around to doing a proper patch which I intend to commit, yes, I'll patch the Makefile.am files of course.

(In reply to comment #3)
> Anyways, on your actual patch - this is why I hate that people build from
> tarballs.  Patches to the Makefile.in are useless.  Make a patch on top of git
> (https://live.gnome.org/GnomeLove/SubmittingPatches) please.

I think what Jasper was after is some inputs of how to implement the js part, _not_ the autoconf part. Obviously once we figure out the best way to implement the js part, he will post a patch against git. The idea was to show what we were currently doing to make it easier for reviewer to understand the issue.
So please, don't look at the Makefile/configure/autoconf part, these are easy to handle.
Anyway, it seems Florian got us the answer we were looking for.

Review of attachment 218742 [details] [review]:

Marking as needs-work based on the above comments.

Please find attached a reworked patch that I'd like to push once it's found to be OK and inline with the coding standards.

Created attachment 222739 [details] [review]
Make NetworkManager support optional

Review of attachment 222739 [details] [review]:

One minor style comment.

::: js/ui/main.js
@@ +44,3 @@
 
+if (Config.HAVE_NETWORK_MANAGER)
+        const NetworkAgent = imports.ui.networkAgent;

Using "const" inside a conditional is a bit weird.  How about:

const NetworkAgent = Config.HAVE_NETWORK_MANAGER ? imports.ui.networkAgent : null;

Yes, that's better indeed. With that tweak ok to push?

I've updated the patch to apply on git master. I'd like to push this diff this week unless someone objects.

Created attachment 225422 [details] [review]
Make NetworkManager support optional

Review of attachment 225422 [details] [review]:

::: configure.ac
@@ +143,3 @@
+AC_MSG_CHECKING([for Network Manager support])
+PKG_CHECK_EXISTS([libnm-glib libnm-util],
+        [ NM_LIBS=`$PKG_CONFIG --libs libnm-glib libnm-util`

Two things are broken here:

1) We need to get the CFLAGS too; it's very common for pkg-config-using libraries to install in a subdirectory of /usr/include to support parallel installation.

2) We're not actually using NM_LIBS in the Makefile.am, so we'll just fail to link.  You need to add NM_CFLAGS to the gnome_shell_cflags variable, and add NM_LIBS to libgnome_shell_la_LIBADD.

In general, since you're splitting nm out of the GNOME_SHELL_CFLAGS and GNOME_SHELL_LIBS variable, you need to update every place that references either one.  For example, a quick "git grep GNOME_SHELL_LIBS" shows it being used in docs/reference/shell/Makefile.am.

@@ +148,3 @@
+	 AC_SUBST([HAVE_NETWORK_MANAGER],[1])
+	 AC_MSG_RESULT([yes])
+	 $network_manager=true],

Drop the $ from the variable when assigning; this is shell, not Perl.

Thanks, I've updated the patch.

Created attachment 225443 [details] [review]
Make NetworkManager support optional

Created attachment 225819 [details] [review]
Make NetworkManager support optional

Fix a syntax error output from configure.

I tried this locally in the enable-NM case; there was a syntax error that happened due to this line

GNOME_SHELL=$GNOME_SHELL $NM_LIBS

which was both conceptually and syntactically wrong.

Otherwise I think this is fine to go in after 3.6.1 is released and gnome-shell opens up for 3.7 development.

Hmm, it turned out the patch wasn't complete as it would still try to load the networkAgent module. We currently "solved" it with this patch:

--- js/ui/sessionMode.js.orig	Mon Oct  8 14:59:38 2012
+++ js/ui/sessionMode.js	Mon Oct  8 15:00:06 2012
@@ -91,7 +91,7 @@ const _modes = {
         isLocked: false,
         isPrimary: true,
         unlockDialog: imports.ui.unlockDialog.UnlockDialog,
-        components: ['networkAgent', 'polkitAgent', 'telepathyClient',
+        components: ['polkitAgent', 'telepathyClient',
                      'keyring', 'recorder', 'autorunManager', 'automountManager'],
         panel: {
             left: ['activities', 'appMenu'],

Though I wonder how to solve this in a proper way. Perhaps appending to _modes.components after declaring it based on config.HAVE_NETWORK_MANAGER ?

Debian also patches Shell to make NetworkManager optional (both for Linux users who don't have/trust NetworkManager, and for kFreeBSD to which NM isn't portable). Our patch appears to originate from a Gentoo developer, and applies to 3.4.x (it hasn't yet been updated for 3.6):

http://anonscm.debian.org/viewvc/pkg-gnome/desktop/unstable/gnome-shell/debian/patches/10-make-NetworkManager-optional.patch?revision=35202&view=markup

It might be useful to compare what's been merged with the patch we were using?

Cross-references: the original bug was <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=652482>; I've opened <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=692049> to track "update the patch for 3.6".

(In reply to comment #19)
> Debian also patches Shell to make NetworkManager optional (both for Linux users
> who don't have/trust NetworkManager, and for kFreeBSD to which NM isn't
> portable). Our patch appears to originate from a Gentoo developer, and applies
> to 3.4.x (it hasn't yet been updated for 3.6):
> 
> http://anonscm.debian.org/viewvc/pkg-gnome/desktop/unstable/gnome-shell/debian/patches/10-make-NetworkManager-optional.patch?revision=35202&view=markup
> 
> It might be useful to compare what's been merged with the patch we were using?
> 
> Cross-references: the original bug was
> <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=652482>; I've opened
> <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=692049> to track "update the
> patch for 3.6".

The patch in https://bugzilla.gnome.org/show_bug.cgi?id=669495 is based on my initial work and was extended by Alexandre Rostovtsev from Gentoo.

Looks to me as if Bug#679871 should be marked as duplicate of #669496

Yes, I think we can close this bug as a dup of 669496 as that has the set of patches that really should go in rather sooner than later.

*** This bug has been marked as a duplicate of bug 669495 ***