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

<lsb-release> reader #1781 #2096

Closed
wants to merge 10 commits into from
Closed

<lsb-release> reader #1781 #2096

wants to merge 10 commits into from

Conversation

surahman
Copy link
Contributor

@surahman surahman commented May 8, 2021

With respect to issue #1781:
I have developed tests and utility routines to read the lsb-release file on Ubuntu-based systems when applicable. When it is unavailable, or parsing fails, there will be a failover to the previous method of using SysInfo::productType() and QSysInfo::productVersion().

@ricab
Copy link
Collaborator

ricab commented May 12, 2021

Hi @surahman, thanks a lot for your work on this!

Unfortunately, we've discussed the disparity between /etc/lsb-release and /etc/os-release and it's not looking like we can rely on any of those. The fact that they're readable at all is misleading.

Instead, we should probably read from /var/lib/snapd/hostfs/etc/os-release and add the system-observe plug to our snap. We'd then have to decide what to do if the user doesn't give us the plug.

We're still discussing details, and we'll get back to you again, but we wanted to let you know so that you don't spend any more time.

@ricab
Copy link
Collaborator

ricab commented May 12, 2021

Hi again @surahman,

So we discussed a bit and confirmed /var/lib/snapd/hostfs/etc/os-release is the way to go, given how snaps are working... The implementation can just return "unknown" when unable to read that file (that's what QSysInfo defaults to as well).

Multipass needs to work on macOS/Windows too, so this would need to be a platform dependent implementation. That can be achieved by declaring a free function in include/multipass/platform.h and implementing it in src/platform/platform_linux.cpp (we'd take care of other platforms). The code you now have for parsing lsb-release would need to be adapted and moved to that compilation unit. The corresponding tests go in tests/linux/test_platform_linux.cpp.

Notice that the Linux implementation should still use QSysInfo when outide of a snap. So, something like

std::string multipass::platform::host_version() // returning single string probably enough for our purposes
{
  return mu::in_multipass_snap() ? 
    our_snap_impl_reading_os_release() :
    fmt::format("{}-{}", QSysInfo::productType(), QSysInfo::productVersion());
}

Let us know if you have questions and thanks again for your involvement!

@surahman
Copy link
Contributor Author

I am on it. I am happy to help and I am learning a lot from your codebase. These interactions and exercises are a great learning/practice tool and it is even better if it is of assistance to someone.

@surahman
Copy link
Contributor Author

I will go ahead and abandon this branch and PR to start on a new branch - it is the cleanest way to revert changes. I will cross-reference the two PR's once I have the new one up.

@surahman
Copy link
Contributor Author

I have created a new branch instead of doing a hard reset on this one: #2105

@Saviq
Copy link
Collaborator

Saviq commented May 13, 2021

Thanks @surahman, I'll close this one then.

@Saviq Saviq closed this May 13, 2021
bors bot added a commit that referenced this pull request Jun 9, 2021
2105: [platform] Read Snap Linux OS Release r=ricab a=surahman

**_Issue #1781:_**
Creating changes requested in #2096 on a new branch instead of doing a hard reset on the original branch to maintain the changes in the original files whilst I work.

Tests have been added to the `PlatformLinux` fixture and an empty value in a field is treated as `unknown`:
```
NAME=""
...
VERSION_ID=""
```
I have not wired into `/src/daemon/daemon.cpp` string pollination because the suggested format of `distro_name-distro_rel` does not match the original.

Co-authored-by: Saad Ur Rahman <saad.ur.rahman@gmail.com>
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.

3 participants