Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show OS info in debug messages #2498

Merged
merged 1 commit into from Oct 3, 2020
Merged

Conversation

codebrainz
Copy link
Member

@codebrainz codebrainz commented May 9, 2020

This PR conditionally uses newer GLib API to get information about the OS and to print it in with the existing debug messages. I don't really have a preference on the formatting of the info, this PR is tested against own-built Geany on Ubuntu 20.04.

Example:

$ /opt/geany/bin/geany -v -c /tmp/cfg0
Geany-INFO: 23:57:45.734: Using alternate configuration directory
Geany-INFO: 23:57:45.785: Geany 1.37 (git >= ea649d802), en_CA.UTF-8
Geany-INFO: 23:57:45.785: GTK 3.24.18, GLib 2.64.2
Geany-INFO: 23:57:45.785: OS Information:
Geany-INFO: 23:57:45.785:   Name       : Ubuntu (ubuntu/focal)
Geany-INFO: 23:57:45.785:   Pretty Name: Ubuntu 20.04 LTS
Geany-INFO: 23:57:45.785:   Version    : 20.04 LTS (Focal Fossa) (20.04)
Geany-INFO: 23:57:45.785: System data dir: /opt/geany/share/geany
Geany-INFO: 23:57:45.785: User config dir: /tmp/cfg0

@elextr
Copy link
Member

elextr commented May 9, 2020

LGBI, can't test ATM due to the requirement for Glib 2.64.

Pending testing it I might suggest making it one line (in printf) "OS: %s %s", name, version and leave Pretty Name off.

But I guess it depends on what it looks like on various systems, for example will Linux Mint be only in Pretty name and Ubuntu be in Name or how else will derived distros be shown? TBD also when anyone can try it on Windows and OSX.

@codebrainz
Copy link
Member Author

Yeah, I've only tested on Ubuntu 20.04 so not sure, but if it's consistent, I agree that name/version is enough or even just pretty name really.

@eht16
Copy link
Member

eht16 commented May 15, 2020

Yay!
I agree also that the pretty name is redundant.

The result of g_get_os_info must be freed even it is not explicitly stated in the docs (https://gitlab.gnome.org/GNOME/glib/-/blob/master/glib/gutils.c#L1507).

Output on ArchLinux:

10:08:55: Geany INFO		: OS Information:
10:08:55: Geany INFO		:   Name       : Arch Linux (arch/)
10:08:55: Geany INFO		:   Pretty Name: Arch Linux
10:08:55: Geany INFO		:   Version    :  ()

Output on Windows 7:

22:50:54: Geany INFO		: OS Information:
22:50:54: Geany INFO		:   Name       : Windows (windows/)
22:50:54: Geany INFO		:   Pretty Name: Windows 7 SP17Vista SP2Vista SP1VistaXP SP3XP SP2XP SP1XP
22:50:54: Geany INFO		:   Version    : 7 SP17Vista SP2Vista SP1VistaXP SP3XP SP2XP SP1XP (7_sp17vista_sp2vista_sp1vistaxp_sp3xp_sp2xp_sp1xp)

The missing version on ArchLinux is OK because it's a rolling release and /etc/os-release doesn't have any VERSION key at all.
I'm not sure about the weird looking version on Windows, either it is just my installation or it usually looks like this ... :)

@codebrainz
Copy link
Member Author

The result of g_get_os_info must be freed even it is not explicitly stated in the docs

I assumed based on the lack of [transfer full] or whatever annotation that it wasn't required, since basically every binding will have a leak otherwise, but you are correct and it's just a bug that the annotation is missing and all automatically generated bindings will leak.

Output on ArchLinux:
Output on Windows 7:

Thanks for testing! Based on the output on Ubuntu, Arch and Windows so far, I suggest we go with only printing the "pretty name", like just this one printf:

geany_debug("OS: %s", get_os_info_field(G_OS_INFO_KEY_PRETTY_NAME));

Sound good?

@techee would it be possible that you could test this on Macos? I'm not sure how involved that would be, if it's too much trouble, no worries.

@eht16
Copy link
Member

eht16 commented May 15, 2020

Sounds good to me to use mainly G_OS_INFO_KEY_PRETTY_NAME. I still would add G_OS_INFO_KEY_VERSION_CODENAME (in braces or so). E.g. for Ubuntu, this gives the version like 20.04 LTS and the codename focal which might be handy and saves some tedious version vs. codename lookups.

@eht16
Copy link
Member

eht16 commented Aug 23, 2020

@codebrainz I guess we could go ahead even without MacOS results.

@codebrainz
Copy link
Member Author

codebrainz commented Aug 29, 2020

I added a commit to update as discussed. Output looks like this now:

Geany-INFO: 07:02:22.879: Geany 1.37 (git >= 27e81be20), en_CA.UTF-8
Geany-INFO: 07:02:22.879: GTK 3.24.20, GLib 2.64.3
Geany-INFO: 07:02:22.879: OS: Ubuntu 20.04.1 LTS (focal)

TODO: squash commits and fast-forward when merging. Done in 6426977

@eht16 eht16 added this to the 1.37 milestone Oct 3, 2020
@techee
Copy link
Member

techee commented Oct 10, 2020

Just for reference, I get

19:29:17: Geany INFO		: GTK 3.24.20, GLib 2.66.0
19:29:17: Geany INFO		: OS: Unknown (Unknown)

on macOS - g_get_os_info() is probably not implemented there.

@codebrainz
Copy link
Member Author

Thanks for testing. You're right, it looks like g_get_os_info() only supports G_OS_INFO_KEY_NAME on Macos.

I guess we could add a check for Macos in our code and make it output something a little more useful.

@techee
Copy link
Member

techee commented Oct 10, 2020

I guess we could add a check for Macos in our code and make it output something a little more useful.

Would be probably better to update glib itself, otherwise we have to include macos headers and link against macos frameworks by ourselves which we don't have to do now. But I don't think it's something too critical to have.

@codebrainz
Copy link
Member Author

Would be probably better to update glib itself...

Definitely would be better to upstream, but probably too much trouble.

But I don't think it's something too critical to have.

Agree. It might be good to just #ifdef this code out on Macos, since it's not working at all.

@elextr
Copy link
Member

elextr commented Oct 10, 2020

Or just #ifdef it to say something like "Unidentified Macos version" so at least that much is obvious.

@codebrainz codebrainz mentioned this pull request Oct 10, 2020
6 tasks
@techee
Copy link
Member

techee commented Oct 12, 2020

Agree. It might be good to just #ifdef this code out on Macos, since it's not working at all.

Or just #ifdef it to say something like "Unidentified Macos version" so at least that much is obvious.

I would just leave it as it is - this is not something worth ifdefs or any extra code IMO.

@codebrainz
Copy link
Member Author

I would just leave it as it is - this is not something worth ifdefs or any extra code IMO.

Too late, I already created follow-up #2618, which puts it in a self-contained utils function, making the main code flow cleaner than in this PR and allowing support for older GLibs and MacOS/others. There was already #ifdef in there, now there's some more, which eventually will be able to mostly be removed. One could argue it's overkill for such a trivial "feature", but Geany's issue tracker has a chronic problem of people not reporting OS/version, so making that easier is, IMO, worth the extra #ifdef-ed code.

@eht16
Copy link
Member

eht16 commented Oct 13, 2020

And since @codebrainz already did the work, let's continue with #2618.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants