I am getting "null" for the time on debug messages with:
``` (null): Geany INFO : Geany 1.39 (git >= b7558417), en_AU.UTF-8 (null): Geany INFO : GTK 3.24.20, GLib 2.64.6 (null): Geany INFO : OS: Linux Mint 20 (ulyana) ```
This issue is to record it as I don't have time to investigate for a while, so if anyone else wants to feel free, IIRC there were some changes around time formatting recently?
When I try to reproduce, it's the debug message _window_ that has no timestamps; strangely, the log output looks fine in the terminal:
~~~ Geany-INFO: 08:01:37.403: Geany 1.39 (git >= 32977676), unknown Geany-INFO: 08:01:37.403: GTK 3.24.5, GLib 2.58.3 Geany-INFO: 08:01:37.403: OS: Linux ~~~
I built from source with `-g -O0` and debugged, without learning the cause, but `g_date_time_format()` is definitely assigning a null pointer to `time_string` before returning from `utils_get_current_time_string()`:
Trying to inspect the `time_string` pointer before it becomes `0x0` raises an error about an invalid address:
Not sure if that points to memory corruption or just the debugger complaining about stripped symbols.
[vscode-gdb-cofig.zip](https://github.com/geany/geany/files/7780230/vscode-gdb-cofig.zip)
@rdipardo Ahh, thanks, yes I was referring to the messages in the `Help->Debug Messages`
@rdipardo ok, thanks for the pointer, problem found 09706bdf8bd61f740a053aa1ea709fc4f65845c5 forces use of `%f` [here](https://github.com/geany/geany/blob/48d4f58966ea81a06e77b9f75079dc989f4636ea...) but clearly only latest Glibses support it.
@eht16 its your change, do you want to revert it or can you somehow test its availability in the runtime Glib?
. . . only latest Glibses support it.
The minimum required version is 2.65.2: https://github.com/frida/glib/blob/6fd4f36bac4d3f9c7ddc809bad6d04bd3365f39b/...
@eht16 its your change, do you want to revert it or can you somehow test its availability in the runtime Glib?
I'm probably oversimplifying the issue, but it seems a tiny patch like this would do the job:
```diff diff --git a/src/utils.c b/src/utils.c index 6f7cb5b0..8283692a 100644 --- a/src/utils.c +++ b/src/utils.c @@ -60,6 +60,11 @@ #include <glib/gstdio.h> #include <gio/gio.h>
+#if GLIB_MAJOR_VERSION >= 0x2 && GLIB_MINOR_VERSION >= 0x41 && GLIB_MICRO_VERSION >= 0x2 +#define G_TIME_FMT_MICRO "%H:%M:%S.%f" +#else +#define G_TIME_FMT_MICRO "%H:%M:%S" +#endif
/** * Tries to open the given URI in a browser. @@ -1050,7 +1055,7 @@ gint utils_parse_color_to_bgr(const gchar *spec) gchar *utils_get_current_time_string(gboolean include_microseconds) { GDateTime *now = g_date_time_new_now_local(); - const gchar *format = include_microseconds ? "%H:%M:%S.%f" : "%H:%M:%S"; + const gchar *format = include_microseconds ? G_TIME_FMT_MICRO : "%H:%M:%S"; gchar *time_string = g_date_time_format(now, format); g_date_time_unref(now); return time_string; ```
The minimum required version is 2.65.2:
Thanks for that.
Your patch looks alright at first glance, except its compile time, not runtime. Newer Geany versions would still show the bug if run on older systems. Thats why I said test availability at runtime.
I said test availability at runtime.
There's `glib_check_version()`, although the spec makes it sound like the target is supposed to be the same version as your own development library (*). ```diff diff --git a/src/utils.c b/src/utils.c index 6f7cb5b0..ecd2c3eb 100644 --- a/src/utils.c +++ b/src/utils.c @@ -1050,7 +1050,8 @@ gint utils_parse_color_to_bgr(const gchar *spec) gchar *utils_get_current_time_string(gboolean include_microseconds) { GDateTime *now = g_date_time_new_now_local(); - const gchar *format = include_microseconds ? "%H:%M:%S.%f" : "%H:%M:%S"; + const gboolean have_f_time_fmt_spec = (glib_check_version(2, 65, 2) == NULL); + const gchar *format = (have_f_time_fmt_spec && include_microseconds) ? "%H:%M:%S.%f" : "%H:%M:%S"; gchar *time_string = g_date_time_format(now, format); g_date_time_unref(now); return time_string; ``` --- (*)
Generally you would pass in the constants GLIB_MAJOR_VERSION, GLIB_MINOR_VERSION, GLIB_MICRO_VERSION as the three arguments to this function; that produces a check that the library in use is compatible with the version of GLib the application or module was compiled against.
https://developer-old.gnome.org/glib/stable/glib-Version-Information.html#glib-check-version
@rdipardo thanks for working on this.
Sorry I wasn't precise enough, need to keep the compile time check _and_ add the default runtime check for newer.
Basically the idea should be to get logic like: ``` if compiled against a new enough version and the runtime version is same or newer (the glib_check_version) then use microseconds else use seconds ``` That ensures the `(null)` doesn't happen by being conservative about which runtime versions are compatible rather than providing microseconds on every system where it is possible. Whilst the microseconds can be useful, we got by without until now, so IMHO its not critical to provide it, but it is critical to always get a time. (@eht16 agree?)
A hackier alternative is to simply try `g_date_time_format()` with `%f` and if it returns `NULL` try without.
Just a comment to the world, Geany doesn't do a general `glib_check_version()`. I'm not sure if its deliberate or if its just nobody ever did it, but its a good thing because it allows Geany compiled against a newer Glib to be run on a machine with an older glib, whereas the `glib_check_version()` can only approve a same or newer version since it doesn't know which functions an application uses and has to assume the ones just added to this version. Of course distros package management restrictions and Windows and OSX having bundled Glib means its unusual to have a mismatch between compile and runtime Glibs so the fact that Geany does not check does no harm in most cases, but allows it if the user wants to try, for example with nightly builds. Sure it might not work, but the dynamic linker should fail if a function is missing so no harm done.
This case gets complicated because we want to provide a workaround, not fail, and the incompatibility is just the contents of a string not a missing function so its really hidden.
Oops, I didn't notice because I used recent enough GLib versions on Windows and Linux :(.
@rdipardo thanks for working on this.
+1
if compiled against a new enough version and the runtime version is same or newer (the glib_check_version) then use microseconds else use seconds
I'm missing something probably but my attempt is way simpler by just checking the GLib version at runtime to decide whether to use `%f` or not. See below.
Whilst the microseconds can be useful, we got by without until now, so IMHO its not critical to provide it, but it is critical to always get a time. (@eht16 agree?)
I fully agree that the log date without microseconds is more important than no log date.
This case gets complicated because we want to provide a workaround, not fail, and the incompatibility is just the contents of a string not a missing function so its really hidden.
I don't see a problem in using `glib_check_version()`, especially in this case where it is as easy as to just modify the string. We don't need compile time checks as there is no API involved.
https://github.com/geany/geany/pull/3076 tries to fix it by using `glib_check_version()` and remove microseconds if GLib version is older than 2.66.0 (which is the relevant stable release after 2.65.2).
@eht16 ok, since there is no API difference (the incompatibility is hidden in a string) you are right, there is no need for a compile time check, #3076 works fine.
Closed #3071 via #3076.
github-comments@lists.geany.org