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

Bump x/sys/unix #118

Merged
merged 1 commit into from Jun 6, 2022
Merged

Bump x/sys/unix #118

merged 1 commit into from Jun 6, 2022

Conversation

akhramov
Copy link
Contributor

@akhramov akhramov commented Jun 5, 2022

A newer version of x/sys/unix updates the OpenBSD bindings
which makes the int8SliceToString helper function obsolete.

This change updates the x/sys/unix version

@@ -2,4 +2,4 @@ module github.com/moby/sys/mountinfo

go 1.16

require golang.org/x/sys v0.0.0-20220412211240-33da011f77ad
require golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably good to update this dependency in all the modules in this repository (mount, mountinfo, symlink, signal)

diff is: golang/sys@33da011...bc2c85a

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this is a good idea.

Comment on lines 60 to 62
mountinfo.Mountpoint = unix.ByteSliceToString(entry.Mntonname[:])
mountinfo.FSType = unix.ByteSliceToString(entry.Fstypename[:])
mountinfo.Source = unix.ByteSliceToString(entry.Mntfromname[:])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

were these renamed at some point ? looks like the current names are;

Suggested change
mountinfo.Mountpoint = unix.ByteSliceToString(entry.Mntonname[:])
mountinfo.FSType = unix.ByteSliceToString(entry.Fstypename[:])
mountinfo.Source = unix.ByteSliceToString(entry.Mntfromname[:])
mountinfo.Mountpoint = unix.ByteSliceToString(entry.F_mntonname[:])
mountinfo.FSType = unix.ByteSliceToString(entry.F_fstypename[:])
mountinfo.Source = unix.ByteSliceToString(entry.F_mntfromname[:])

https://github.com/golang/sys/blob/bc2c85ada10aa9b6aa9607e9ac9ad0761b95cf1d/unix/ztypes_openbsd_amd64.go#L99-L102

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I see; that's for the other bsd types, but for darwin (macOS) and freebsd it's the previous names

so you'll need multiple implementations for this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks, good catch.

Comment on lines 16 to 22
func getMountinfo(entry *unix.Statfs_t) *Info {
var mountinfo Info
mountinfo.Mountpoint = int8SliceToString(entry.F_mntonname[:])
mountinfo.FSType = int8SliceToString(entry.F_fstypename[:])
mountinfo.Source = int8SliceToString(entry.F_mntfromname[:])
return &mountinfo
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above; instead of removing this file, it's still needed to have separate implementations, but you can remove the int8SliceToString() function,

Perhaps we should also remove the intermediate mountinfo variable, and just return a literal (looks slightly cleaner);

Suggested change
func getMountinfo(entry *unix.Statfs_t) *Info {
var mountinfo Info
mountinfo.Mountpoint = int8SliceToString(entry.F_mntonname[:])
mountinfo.FSType = int8SliceToString(entry.F_fstypename[:])
mountinfo.Source = int8SliceToString(entry.F_mntfromname[:])
return &mountinfo
}
func getMountinfo(entry *unix.Statfs_t) *Info {
return &Info{
Mountpoint: unix.ByteSliceToString(entry.F_mntonname[:]),
FSType: unix.ByteSliceToString(entry.F_fstypename[:]),
Source: unix.ByteSliceToString(entry.F_mntfromname[:]),
}
}

@akhramov akhramov force-pushed the feature/bump-x-sys branch 2 times, most recently from 9728999 to 2e48ad7 Compare June 5, 2022 16:02
FSType: unix.ByteSliceToString(entry.F_fstypename[:]),
Source: unix.ByteSliceToString(entry.F_mntfromname[:]),
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missed a newline here

mountinfo.FSType = unix.ByteSliceToString(entry.Fstypename[:])
mountinfo.Source = unix.ByteSliceToString(entry.Mntfromname[:])
return &mountinfo
return &Info{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter is complaining about this part; I haven't checked locally, but possibly indentation is wrong in this file?

mountinfo_freebsdlike.go:9: File is not `gofumpt`-ed (gofumpt)
[18](https://github.com/moby/sys/runs/6746317554?check_suite_focus=true#step:4:19)
     return &Info{

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, absolutely. Sorry for the hustle.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@thaJeztah
Copy link
Member

@kolyshkin PTAL

@kolyshkin

This comment was marked as outdated.

@@ -3,3 +3,5 @@ github.com/moby/sys/mountinfo v0.6.1/go.mod h1:3bMD3Rg+zkqx8MRYPi7Pyb0Ie97QEBmdx
golang.org/x/sys v0.0.0-20211025201205-69cdffdb9359/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220412211240-33da011f77ad h1:ntjMns5wyP/fN65tdBD4g8J5w8n015+iIIs9rtjXkY0=
golang.org/x/sys v0.0.0-20220412211240-33da011f77ad/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a h1:dGzPydgVsqGcTRVwiLJ1jVbufYwmzD3LfVPLKsKg+0k=
golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you forgot go mod tidy (and our CI do not have a check for that).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I was wondering that; got a different result locally, but thought it would be different Go version (as CI was green)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, thanks

@kolyshkin

This comment was marked as outdated.

A newer version of x/sys/unix updates the OpenBSD bindings
which makes the `int8SliceToString` helper function obsolete.

This change updates the x/sys/unix version

Signed-off-by: Artem Khramov <akhramov@pm.me>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@kolyshkin kolyshkin merged commit 1bf36f7 into moby:main Jun 6, 2022
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

3 participants