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

better OS version parsing #433

Closed
glimchb opened this issue Jul 1, 2024 · 8 comments · Fixed by #439
Closed

better OS version parsing #433

glimchb opened this issue Jul 1, 2024 · 8 comments · Fixed by #439
Labels
good first issue Good for newcomers

Comments

@glimchb
Copy link
Member

glimchb commented Jul 1, 2024

see

osName := replaceQuotes(strings.Split(linesInFileContains(OS_RELEASE_FILE, "NAME"), "=")[1])
osVersion := replaceQuotes(strings.Split(linesInFileContains(OS_RELEASE_FILE, "VERSION"), "=")[1])

maybe better use:

import "github.com/go-ini/ini"
cfg, err := ini.Load("/etc/os-release")
cfg.Section("").Key("VERSION").String()

or existing go constants... like runtime.GOOS...

@glimchb glimchb added the good first issue Good for newcomers label Jul 1, 2024
@glimchb
Copy link
Member Author

glimchb commented Jul 1, 2024

@Xeckt wanna take this ?

@Xeckt
Copy link
Contributor

Xeckt commented Jul 3, 2024

I can take this on. Will have to tackle it somewhat later as we're finishing a project at work.

@Xeckt
Copy link
Contributor

Xeckt commented Jul 5, 2024

@glimchb - How would you like this done? Because if we want to fetch OS, do we want to create a separate util function for it in the event it needs to be handle elsewhere in the future? That way we can make the logic a little longer so it's more reliable to parse.

Is it also necessary to parse version? For example Arch Linux os-release file does not contain version so the above code would error out.

@Xeckt
Copy link
Contributor

Xeckt commented Jul 5, 2024

I don't know how important the name of the OS being specific to a distribution is. A simple, reliable solution could be to syscall uname. Something like this (just a working example I came up with):

func GetOs() {
	var u syscall.Utsname
	if err := syscall.Uname(&u); err != nil {
		log.Fatal(err)
	}
	fmt.Println(
		utsToString(u.Machine),
		utsToString(u.Release),
                utsToString(u.Sysname),
	)
}

func utsToString(uname [65]int8) string {
	var b [65]byte
	for i := range uname {
		b[i] = uint8(uname[i])
	}
	return string(b[:])
}

Gives on my Arch linux:

Architecture: x86_64 (u.machine)
Version: 6.9.6-arch1-1 (u.release)
Type: Linux (u.sysname)

I don't know how important having the exact distribution string on here is though, maybe for debug purposes later.

I can of course clean up the above to work as a single function as well.

@glimchb
Copy link
Member Author

glimchb commented Jul 5, 2024

For now i think we should go with simple improvement

import "github.com/go-ini/ini"
cfg, err := ini.Load("/etc/os-release")
cfg.Section("").Key("VERSION").String()

@Xeckt
Copy link
Contributor

Xeckt commented Jul 6, 2024

That's fine but we can do it with built in packages over adding more dependencies?

Plus VERSION keys don't always exist on os-release files as mentioned before, what would you like to do about that? Obviously we can nil check, but if version is needed?

@glimchb
Copy link
Member Author

glimchb commented Jul 6, 2024

replaceQuotes(strings.Split(linesInFileContains(OS_RELEASE_FILE, "NAME"), "=")[1])

this looks ugly

if err := syscall.Uname(&u); err != nil {

I don't ike syscalls here, espcially if we run inside docker, we can easily map os-release file inside container

Plus VERSION keys don't always exist on os-release files

understood, we can deal with this when requirement will come up

@Xeckt
Copy link
Contributor

Xeckt commented Jul 11, 2024

Mapping os-release seems to add another layer of logic that doesn't seem needed? Though I like the idea of mapping inside a container, which begs the questions:

  1. Does this mean that the user will have to map the file themselves? If you are adamant about using os-release?
  2. Is it necessary to have os information inside the container about the host, if it's running in a container? Isn't it better applicable to have os information about the isolated environment it will actually run in?
  3. Can we not just use syscalls for the host and then manually move the pulled out information to the container if it's an absolute requirement to have host info? If not, then how about reading the os-release file and carrying the info over into the container without mapping it?

Syscalls just seem the more reliable way in my eyes. I understand /etc/os-release is present on all linux machines, and if you want to fetch the real distribution name instead, then it does make sense. There are prettier ways to do it than the current without dependencies. But as far as I'm concerned, knowing the os, the architecture, and kernel version is enough for whatever you may use it for.

@glimchb glimchb linked a pull request Jul 23, 2024 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants