Created attachment 279147 [details] [review]
GObject: warn on use of deprecated properties

Review of attachment 279147 [details] [review]:

Looks ok to me.

Attachment 279147 [details] pushed as d0e7061 - GObject: warn on use of deprecated properties

Created attachment 279431 [details] [review]
GObject: tweak property deprecation warnings

Don't emit property deprecation warnings for construct properties that
are being set to their default value during construction, but _do_ emit
them in all cases when the property was explicitly given to
g_object_new().

Review of attachment 279431 [details] [review]:

looks good

Comment on attachment 279431 [details] [review]
GObject: tweak property deprecation warnings

Attachment 279431 [details] pushed as 6677906 - GObject: tweak property deprecation warnings

This caused some installedtests failures:

clutter: http://build.gnome.org/continuous/buildmaster/builds/2014/06/29/21/integrationtest/work-gnome-continuous-x86_64-runtime/installed-test-results/clutter_actor-anchors.test/output.txt

ostree: http://build.gnome.org/continuous/buildmaster/builds/2014/06/29/21/integrationtest/work-gnome-continuous-x86_64-runtime/installed-test-results/ostree_test-admin-deploy-2.test/output.txt

I think what would be ideal here is some way to use -DGLIB_VERSION_MAX_ALLOWED to only warn if my app is using properties that are deprecated in a newer version.  Unfortunately I can't think of a way to do that, since properties are runtime, not compile time.

There's also the aspect that libraries like clutter want to be able to test their own deprecated properties.

For now I'll fix ostree.

I've been thinking about this a bit more and it's not something that I feel totally comfortable just dismissing.

I think we need a better policy about the meaning/purpose of error/critical/warning/message/debug in GLib, along with some ground rules.

I'd say something like:

  error: always fatal

  critical: programmer error -- by definition, addition of a new critical is
            a new restraint on the programmer and therefore should be
            considered an API change

  warning: an error outside of the control of the program and therefore
           to be expected at any time.  as such, we can add new warnings
           as we please.

  message: ??

  debug: not shown by default


We should couple this with the ability to ignore all warnings in gtest.

Maybe another approach here is that these sort of deprecation messages should be in the (vastly underused) g_message() category and we ignore those in testcases....

I like the idea of moving it to message, although we should add some structure around it so that tools can see that the message is deprecation.

Could be as low tech as g_message ("[deprecated] ...") or so.

Maybe [deprecated property] for more structure.

Incidentally, I can't (easily) fix ostree because the new soup_server_listen() API is in a newer version of libsoup than I can depend on.  

I could detect the max allowed version at compile time and use #ifdef I guess.

(In reply to comment #7)
> This caused some installedtests failures:
> 
> clutter:
> http://build.gnome.org/continuous/buildmaster/builds/2014/06/29/21/integrationtest/work-gnome-continuous-x86_64-runtime/installed-test-results/clutter_actor-anchors.test/output.txt

I added G_ENABLE_DIAGNOSTIC=0 to the conformance tests, but I didn't think of the installed tests. Matthias put `env G_ENABLE_DIAGNOSTIC=0` in the installed test launcher, so I should probably do the same. it'd be nice if the installed tests launcher would allow this to happen automatically unless overridden.

Not a huge fan of hardcoding glib environment variables in g-d-t-r; while it does start with "gnome-desktop" it's conceptually independent - it's a reference/demo implementation of the InstalledTests spec.

So I think that leaves us with per-component usage of G_ENABLE_DIAGNOSTIC, or something else.

(In reply to comment #11)
> Not a huge fan of hardcoding glib environment variables in g-d-t-r; while it
> does start with "gnome-desktop" it's conceptually independent - it's a
> reference/demo implementation of the InstalledTests spec.

we could add a new key to the .test file, `TestEnvironment`:

  [Test]
  Type=session
  TestEnvironment=G_ENABLE_DIAGNOSTIC=0;CLUTTER_ENABLE_DIAGNOSTIC=0;
  Exec=/usr/libexec/installed-tests/clutter/actor-anchor

this would allow you to control the execution environment of the tests for other use cases as well.

Created attachment 284801 [details] [review]
runner: Allow overriding test environment

In the fallout of https://bugzilla.gnome.org/show_bug.cgi?id=732184
tests exercising deprecated properties are now failing.

When running under `make check`, we can change the environment of a test
by using the TEST_ENVIRONMENT variable. The .test file that we use to
describe a test should have the same facility: a `TestEnvironment` key
that contains a list of key=value pairs that get set in the environment
prior to execution.

gnome-desktop-testing does not have a bugzilla product, so I attached the patch here. it would be good if somebody could review it.

Review of attachment 284801 [details] [review]:

Two minor things, otherwise looks fine and please commit.

::: src/gnome-desktop-testing-runner.c
@@ +136,3 @@
   const char *test_path;
+  char **env_key = NULL;
+  GError *internal_error;

= NULL please, per the rest of the code style.

@@ +174,3 @@
     }
+
+  internal_error = NULL;

(And not here)

@@ +180,3 @@
+        internal_error->code == G_KEY_FILE_ERROR_KEY_NOT_FOUND))
+    goto out;
+  else

g_clear_error (&internal_error);

Comment on attachment 284801 [details] [review]
runner: Allow overriding test environment

pushed, with the changes as instructed.

For ostree, I chose not to use the environment variable approach, and instead did:

https://git.gnome.org/browse/ostree/commit/?id=34c336c1f3ad98918bb044533a43985413d05734

Why should this be on by default?

The message is directed at developers, not users, and doesn't represent
a bug.  Users, in fact, really cannot do anything useful with a scary-
looking message like that.

What really happens here is that application writers lose control over
their stderr.

> What really happens here is that application writers lose control over
> their stderr.

Is should stress that this includes command line utilities that happen to
use glib/gobject.  Gnumeric's "ssconvert" is one such utility.

Having subsequently installed versions of glib/gobject/gtk+/etc. start
spewing error messages because something else got deprecated is really
upsetting.

I'm finding this to be pretty ugly and painful too.

The thing is that GLib is quite good now about deprecations with the compile-time min/max version stuff - love that change.  These runtime warnings don't respect what compile-time options I used when building my app, and end up being shown to users and admins, who just don't care.

What was the rationale behind this?  What deprecated properties did we want people to see?  Can we have this only on for Continuous, or only on during devel cycles?

It was the schema / schema_id change in GSettings that prompted this warning.

I think it's fine to say that it's only on in dev cycles, but even then, it's pretty shit -- anything that does any bit of introspection like the GTK+ inspector or GJS will hit this a lot.

I'd rather just disable it globally.

I'll throw in another voice in support of the idea that we shouldn't nag our users to nag our developers to fix deprecations :-) 

In general, if users see warnings in logs or standard out they expect they represent problems; a deprecation is specifically *not* a problem - we're retaining some old way of working for compatibility. (And I don't mean to say that users would think differently if they were "messages" rather than "warnings".)

If we tagged property deprecations with version, then we could think of some way of checking deprecations against a declared version at compile time - since we don't, I think diagnostics about them have to be dependent on some sort of "debug mode", which I think G_ENABLE_DIAGNOSTIC was meant to be.

One idea I have is that we have a one-time message about enabling G_ENABLE_DIAGNOSTIC in your ~/.bashrc that we print out the first time there's a critical warning if G_ENABLE_DIAGNOSTIC is set (and stdin is a tty?). It wouldn't *actually* be because the diagnostic would help to figure out the critical warning - it would be that developers inevitably hit critical warnings and try to track them down.

Re-opening to get back on Ryan's and Matthias' radars.

You're a bit late to the party.  This is already fixed since last week on the stable branch:

  https://git.gnome.org/browse/glib/commit/?h=glib-2-42&id=b12bd1c3dcfbb398d2462dcf584a1f6d5173ca9a

Still doesn't fix the "open GtkInspector, see giant spam of warnings" issue, nor does it fix code that validly uses the max-version / min-version defines or the IGNORE_DEPRECATIONS macros.

*** Bug 737065 has been marked as a duplicate of this bug. ***

*** Bug 733298 has been marked as a duplicate of this bug. ***

*** Bug 751266 has been marked as a duplicate of this bug. ***

(In reply to Owen Taylor from comment #22)
 
> One idea I have is that we have a one-time message about enabling
> G_ENABLE_DIAGNOSTIC in your ~/.bashrc that we print out the first time
> there's a critical warning if G_ENABLE_DIAGNOSTIC is set (and stdin is a
> tty?). It wouldn't *actually* be because the diagnostic would help to figure
> out the critical warning - it would be that developers inevitably hit
> critical warnings and try to track them down.

If only that were true. Sadly, reality has been that application developers simply ignore critical warnings at run time, and in various cases even warnings at compile time, if they don't disable them outright.

Ideally, users that get run time warnings would report them to the application, but since those warnings come prefixed with the GLib/GTK domains, we get the reports instead, and then we have to tell users to go and file bugs were appropriate.

We cannot ship two different versions of our libraries — one for application developers, and one for users. I mean, we can ship stable releases of our platform libraries as part of a Flatpak run time, these days, and those could be built without deprecation warnings for properties and signals; of course, application developers building against those Flatpak run times would also not see any warning, and we'd be back to square one.

It would be great if we had a way to enable these warnings only for application developers, but the truth of the matter is:

 - application developers simply don't run development snapshots of our libraries
 - left to their own devices, application developers will disable any warning coming from our platform
 - very few applications are actually employing continuous integration and functional tests, so they will never see a run time warning until somebody actually runs the code; this means that "enabling only on CI" is not going to work, and it would require running an undefined number of applications under CI to begin with

These are hard won truths gathered over the past 15 years, and they make any solution that satisfies everyone pretty much impossible to achieve.

I'd close this bug and just add a documentation note saying that you should set `G_ENABLE_DIAGNOSTIC` in your environment when updating your GLib-based dependencies, in order to catch deprecation warnings at run time.

An additional thing we ought to consider is to ensure that GNOME Builder enables diagnostic warnings, so that new developers will not form the habit of ignoring them. We can't do anything for older developers, and developers using other tools, but we can't win all battles either.

> Sadly, reality has been that application developers
> simply ignore critical warnings at run time

I'll call that the gtk+ developer's reality.

The application developer's reality is that most criticals at runtime
are due to gtk+ changes after the application was released.  "ignoring"
doesn't really enter into the situation.

The Truth[tm] is probably somewhere in-between and with lots of variation.

(In reply to Morten Welinder from comment #31)
> > Sadly, reality has been that application developers
> > simply ignore critical warnings at run time
> 
> I'll call that the gtk+ developer's reality.

Call it whatever you want, but this is the GLib bug tracker, so I'll discuss the reality that we, as platform developers, are facing.

> The application developer's reality is that most criticals at runtime
> are due to gtk+ changes after the application was released.

And that would be fine and good if you thought that the platform you're consuming won't ever deprecate anything or add anything — and that platform developers are not allowed to tell you to change your code to drop deprecated functionality; which is a perfectly valid position, mind you, and if that's what you want I strongly endorse you start bundling your dependencies, to avoid platform developers releasing new versions of their projects and thus tarnishing your users' terminals with warnings. This way, you control the dependencies, and their behaviour.

(In reply to Emmanuele Bassi (:ebassi) from comment #29)
> I'd close this bug and just add a documentation note saying that you should
> set `G_ENABLE_DIAGNOSTIC` in your environment when updating your GLib-based
> dependencies, in order to catch deprecation warnings at run time.

Do you want to put a patch together for that? You obviously have a fairly good idea of how you want it to look/where you want it to be.

(In reply to Emmanuele Bassi (:ebassi) from comment #30)
> An additional thing we ought to consider is to ensure that GNOME Builder
> enables diagnostic warnings, so that new developers will not form the habit
> of ignoring them. We can't do anything for older developers, and developers
> using other tools, but we can't win all battles either.

I’ve filed bug #790761.

Created attachment 368198 [details] [review]
docs: Add G_ENABLE_DIAGNOSTIC

We need to mention this environment variable, so that developers have a
fighting chance of actually using it.

Review of attachment 368198 [details] [review]:

That looks good. It’s helpfully already mentioned in the documentation for G_PARAM_DEPRECATED and G_SIGNAL_DEPRECATED.

Attachment 368198 [details] pushed as 9453b97 - docs: Add G_ENABLE_DIAGNOSTIC