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

SysInfo refactoring, startup log update #4811

Merged
merged 23 commits into from Jun 7, 2014

Conversation

Karlson2k
Copy link
Member

Started as small startup log refactoring, this branch growed up to SysInfo refactoring and cleanup.
I fixed a lot of mess and inconsistency in SysInfo, removed some outdated and platform-specific functions.
New function are designed to be used in platform-agnostic way.

@Karlson2k
Copy link
Member Author

jenkins build this please

@Karlson2k
Copy link
Member Author

jenkins build this please

@Karlson2k
Copy link
Member Author

@jmarshallnz Would you like to review? :)

@jmarshallnz
Copy link
Contributor

Looks like you need to fixup atv/ios

@Karlson2k
Copy link
Member Author

@jmarshallnz Thanks, that was stupid c/p error. :)
Fixed.
BTW darwin builds again missing NDEBUG define. Seems this includes Gotham release.

jenkins build this please

@Memphiz
Copy link
Member

Memphiz commented May 29, 2014

darwin builds have ndebug in release mode only - see

https://github.com/xbmc/xbmc/blob/master/tools/darwin/Configurations/Release.xcconfig

@Karlson2k
Copy link
Member Author

@Memphiz xbmclogs full of logs with "Using Unknown XBMC x32 build" on darwin.
Do we have other choice than Release and Debug?

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone May 29, 2014
@MilhouseVH
Copy link
Contributor

Seems to work fine on Raspberry Pi (OpenELEC).

Before PR4811 log
After PR4811 log

The only noticeable change on the System Info page (Confluence) is the change of operating system name from:
Linux 3.14.4 #1 PREEMPT Thu May 29 02:25:30 BST 2014 armv6l GNU/Linux
to
OpenELEC_Helix 0.0 (kernel: Linux ARM 32-bit version 3.14.4)
Note sure what the "0.0" following OpenELEC_Helix is about though.

@stefansaraev
Copy link
Contributor

@MilhouseVH probably something we miss in os-release. thanks for pointing it. I'll take a look.

@Karlson2k
Copy link
Member Author

@stefansaraev @MilhouseVH Right, you need it in /etc/os-release

@MilhouseVH
Copy link
Contributor

Just a guess, but it probably needs to be the OE git hash or something that uniquely identifies the OE build as neither the log or Sys Info screen identify the OE build beyond "OpenELEC_Helix" where as before the log at least included the full OE name (build date, time, revision, git hash).

@stefansaraev
Copy link
Contributor

@MilhouseVH I'd like to move the non-related openelec discussion in irc, to avoid spaming others. I'll check whats required and do a PR for OE, or ping you in irc.

@stefansaraev
Copy link
Contributor

ok @Karlson2k I believe there is something wrong in os-release parser, getValueFromOs_release.
in fact, it doesnt work even with ubuntu's shipped os-release (14.04). and it even says "Unknown Linux Distribution", if I move VERSION_ID= on TOP of the file (first line, so NAME= is second). hum.

@Karlson2k
Copy link
Member Author

@stefansaraev Thanks, I'll check it

@Karlson2k
Copy link
Member Author

@stefansaraev Did you disable lsb-release parser getValueFromLsb_release() for your tests?
It must work as fallback and return correct values when /etc/os-release parser fails. lsb-release parser works fine on my Ubuntu.

@stefansaraev
Copy link
Contributor

nope. I did not. our lsb_release returns nonsense anyway ;) it doesnt support -v/-c/etc.
tbh I didnt expect the parser to fail reading perfectly valid os-release file

EDIT: hint for a simple os-release parser: https://lkml.org/lkml/2012/9/5/419 maybe this helps.

EDIT2: to be clear, I did not test on ubuntu. I tested on openelec with /etc/os-release c/p'd from ubuntu (I thought there is something wrong with OE's os-release, which is fine, except not wrapping the values with "")

@Karlson2k
Copy link
Member Author

@stefansaraev Thanks, I fixed this bug.
Should works well now.

jenkins build this please

@Karlson2k
Copy link
Member Author

@stefansaraev According to http://www.freedesktop.org/software/systemd/man/os-release.html you must quote values if they contain spaces.
os-release must be compatible with bash syntax as it may be included in scripts like

#!/bin/bash
. /etc/os-release

@MilhouseVH
Copy link
Contributor

Building OpenELEC with the latest commits and I'm getting this build error:

Application.cpp: In member function 'virtual bool CApplication::Create()':
Application.cpp:719:118: error: request for member 'c_str' in 'g_sysinfo.CSysInfo::GetUsedCompilerNameAndVer', which is of non-class type 'std::string() {aka std::basic_string<char>()}'
   CLog::Log(LOGNOTICE, "XBMC compiled " __DATE__ " by %s for %s %s %d-bit %s (%s)", g_sysinfo.GetUsedCompilerNameAndVer.c_str(), g_sysinfo.GetBuildTargetPlatformName().c_str(),
                                                                                                                      ^
/home/neil/projects/OpenELEC.tv/build.OpenELEC_Helix-RPi.arm-devel/xbmc-master-14-5ec51aa/Makefile.include:93: recipe for target 'Application.o' failed
make[2]: *** [Application.o] Error 1
make[2]: *** Waiting for unfinished jobs....

…ll platforms (currently only Darwin, Win32 and Android return meaningful values)
…OS_VERSION_INFO

Old name was incorrect as XBMC provide much more information than just kernel version
@Karlson2k
Copy link
Member Author

@t-nelson Lowered number of commits.

@Memphiz
Copy link
Member

Memphiz commented Jun 4, 2014

@Karlson2k seems i am blind - darwin sets _DEBUG (with underscore) of course. Seems impossible to me that "buildType = "Unknown";" could be triggered in Application.cpp with the code i am seeing in that PR here. wonders

@Karlson2k
Copy link
Member Author

@Memphiz See xbmclogs.com - a lot of Gotham logs wtih "Unknown" type.

@Memphiz
Copy link
Member

Memphiz commented Jun 4, 2014

@Karlson2k thx for the headsup - found the problem - pr incoming...

@Karlson2k
Copy link
Member Author

This is the reason why I always add third case when only two choices must be valid. :)

@Memphiz
Copy link
Member

Memphiz commented Jun 4, 2014

@Karlson2k for reference #4851

@MartijnKaijser
Copy link
Member

good for merge (after jenkins build) or does it need some squashing?

@Karlson2k
Copy link
Member Author

@MartijnKaijser It's squashed enough and ready for merge.

@Karlson2k
Copy link
Member Author

jenkins build this please just in case

@Karlson2k
Copy link
Member Author

Anyone want to review? @jmarshallnz ? :)

@jmarshallnz
Copy link
Contributor

Looks OK to me on the whole. Didn't check all the hundreds of little changes though for sanity/null check/running off array. I'm sure you'll fix them if there are any that snuck in.

jmarshallnz added a commit that referenced this pull request Jun 7, 2014
SysInfo refactoring, startup log update
@jmarshallnz jmarshallnz merged commit 441191d into xbmc:master Jun 7, 2014
@MartijnKaijser MartijnKaijser modified the milestones: Helix 14.0-alpha1, Pending for inclusion Jun 7, 2014
@Karlson2k Karlson2k deleted the startup_log_04 branch June 7, 2014 20:36
@Karlson2k Karlson2k mentioned this pull request Jun 9, 2014
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

7 participants