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

Improve detection speed for LSB based systems #28

Merged
merged 3 commits into from
Feb 22, 2013

Conversation

roehling
Copy link
Contributor

The new code avoids calling the external lsb_release
binary and reads the /etc/lsb-release issue file instead.

The old code called lsb_release quite excessively:

  • once in is_os()
  • twice in get_version() and get_codename()

On Ubuntu, this totals to 7 popen calls just to detect the OS.

The new code avoids calling the external lsb_release
binary and reads the /etc/lsb-release issue file instead.
@wjwwood
Copy link
Contributor

wjwwood commented Feb 21, 2013

Code and logic looks fine, but we should test it on lucid and quantal to ensure that nothing is different about the contents in /etc/lsb-release.

@roehling
Copy link
Contributor Author

My work computer is quantal, and my file server at home is lucid. Both have the appropriate content in /etc/lsb-release.

@wjwwood
Copy link
Contributor

wjwwood commented Feb 21, 2013

Ok, sounds good to me then, @tfoote you want to take a look too?

@tfoote
Copy link
Member

tfoote commented Feb 21, 2013

It looks like there's an lsb_release module so we don't have to do our own parsing:

>>> import lsb_release
>>> lsb_release.get_lsb_information()
{'RELEASE': '10.04', 'CODENAME': 'lucid', 'ID': 'Ubuntu', 'DESCRIPTION': 'Ubuntu 10.04.4 LTS'}

@roehling
Copy link
Contributor Author

According to https://bugs.launchpad.net/rester/+bug/789736 we cannot assume that the lsb_release module is available on all systems. They suggest the platform module instead.

@tfoote
Copy link
Member

tfoote commented Feb 21, 2013

I just ran into that too. It does appear the platform module does what we need and is more standard.

@roehling
Copy link
Contributor Author

I tested the platform module code with Ubuntu and Debian, but I have no access to Linux Mint. As you can see, I had to change the Debian identifier to lower-case, so it might break on Linux Mint as well.

tfoote added a commit that referenced this pull request Feb 22, 2013
@tfoote tfoote merged commit 0fb45c4 into ros-infrastructure:master Feb 22, 2013
@roehling roehling deleted the lsbfix branch February 22, 2013 19:36
@jbohren
Copy link
Contributor

jbohren commented Mar 12, 2013

Any chance a new version of rospkg with this fix could get released?

@tfoote
Copy link
Member

tfoote commented Mar 13, 2013

This merge broke the unit tests. I think that #32 has everything required left. We're starting to transition toward requiring reviewed pull requests for commits to release branches. If you want to review #32 and +1 it for merging that would help.

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