/snapd

release,cmd,dirs: Redo the distro checks to take into account distribution families #3984

Merged
merged 4 commits into from Oct 4, 2017

Conversation

Reviewers

@mvo5 mvo5

@zyga zyga

@chipaca chipaca

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@Conan-Kudo @codecov-io @zyga @mvo5 @chipaca
@Conan-Kudo
Contributor

Conan-Kudo commented Sep 28, 2017

This PR changes the distribution checks in snapd to account for distribution families, so that snapd behaves as expected across not only the main distributions we support, but their derivatives, too.
@zyga

zyga requested changes Sep 28, 2017
View changes

A few comments, thank you for pushing this.

cmd/cmd.go
@@ -66,8 +66,17 @@ func distroSupportsReExec() bool {
if !release.OnClassic {
return false
}
- switch release.ReleaseInfo.ID {
- case "fedora", "centos", "rhel", "opensuse", "suse", "poky", "arch", "manjaro", "lirios":
+ var contains = func(haystack []string, needle string) bool {
@zyga

zyga Sep 28, 2017

Contributor

Can you please move this to strutil.Contains?

@Conan-Kudo

Conan-Kudo Sep 28, 2017

Contributor

Hmm, apparently this already exist as strutil.ListContains, I'll just reuse that.

@zyga

zyga Sep 29, 2017

Contributor

Ah, perfect.

cmd/cmd.go
+ return false
+
+ }
+ switch {
@zyga

zyga Sep 28, 2017

Contributor

The switch makes little sense here, can you make that into a normal if statement now please?

@Conan-Kudo

Conan-Kudo Sep 28, 2017

Contributor

This is still checking more than one conditional, so the switch makes sense still, I think.

dirs/dirs.go
@@ -165,8 +165,17 @@ func SetRootDir(rootdir string) {
}
GlobalRootDir = rootdir
- switch release.ReleaseInfo.ID {
- case "fedora", "centos", "rhel", "arch", "manjaro", "lirios":
+ var contains = func(haystack []string, needle string) bool {
@zyga

zyga Sep 28, 2017

Contributor

Once you move the other function please just reuse it here.

release/release_test.go
@@ -93,6 +93,35 @@ BUG_REPORT_URL="https://bugs.launchpad.net/elementary/+filebug"`
c.Check(os.VersionID, Equals, "0.4")
}
+func (s *ReleaseTestSuite) TestFamilyOSRelease(c *C) {
+ mockOSRelease := filepath.Join(c.MkDir(), "mock-os-release")
@zyga

zyga Sep 28, 2017

Contributor

Nice test case

release/release_test.go
+func (s *ReleaseTestSuite) TestFamilyOSRelease(c *C) {
+ mockOSRelease := filepath.Join(c.MkDir(), "mock-os-release")
+ dump := `NAME="CentOS Linux"
+ VERSION="7 (Core)"
@zyga

zyga Sep 28, 2017

Contributor

I think you have to unindent this. Alternatively just move the variable dump out of the function (and make it a const)

@Conan-Kudo
Contributor

Conan-Kudo commented Sep 28, 2017

@zyga This should address all your feedback now. 👍
@Conan-Kudo
release: Add support for processing ID_LIKE from os-release(5) 
This will allow us to handle distribution families better.

Signed-off-by: Neal Gompa <ngompa13@gmail.com>
4ea282e
@Conan-Kudo
Contributor

Conan-Kudo commented Sep 29, 2017

I've rebased it against current master to resolve conflicts.
@zyga

zyga requested changes Sep 29, 2017 • edited
View changes

I think it is close, let me know if you want to help with tests. Feel free to take stuff from https://github.com/zyga/os-release-zoo

cmd/cmd.go
- switch release.ReleaseInfo.ID {
- case "fedora", "centos", "rhel", "opensuse", "suse", "poky", "arch", "manjaro", "lirios", "solus":
+ switch {
+ case release.ReleaseInfo.ID != "debian", !strutil.ListContains(release.ReleaseInfo.IDLike, "debian"):
@zyga

zyga Sep 29, 2017

Contributor

Can you please add a similar check for ID_LIKE=ubuntu, the code as it stands today would regress on Elementary OS

NAME="elementary OS"
VERSION="0.4 Loki"
ID="elementary OS"
ID_LIKE=ubuntu
PRETTY_NAME="elementary OS Loki"
VERSION_ID="0.4"
HOME_URL="http://elementary.io/"
SUPPORT_URL="http://elementary.io/support/"
BUG_REPORT_URL="https://bugs.launchpad.net/elementary/+filebug"
release/release_test.go
+ c.Check(os.VersionID, Equals, "7")
+ c.Check(os.IDLike, DeepEquals, []string{"rhel", "fedora"})
+}
+
@zyga

zyga Sep 29, 2017

Contributor

I'd like to see a few more test cases for distributions:

  • fedora
  • centos (thanks)
  • rhel
  • ubuntu
  • ubuntu core
  • elementary
  • mint
  • arch

etc.

What we save by having nice generic code here I'd like to check by having very comprehensive test suite that measures behaviour.

@mvo5

mvo5 Sep 29, 2017

Collaborator

AIUI @zyga has a hole bunch of examples of those maybe we can import them here?

@zyga

zyga Sep 29, 2017

Contributor

Yes, I linked to them above.

@Conan-Kudo

Conan-Kudo Sep 29, 2017

Contributor

I don't want to import all of them, as that isn't quite necessary. We just need to prove that the negative cases work.

@mvo5

mvo5 reviewed Sep 29, 2017
View changes

Looks good, some ideas inline, thanks a bunch for working on this.

cmd/cmd.go
- switch release.ReleaseInfo.ID {
- case "fedora", "centos", "rhel", "opensuse", "suse", "poky", "arch", "manjaro", "lirios", "solus":
+ switch {
+ case release.ReleaseInfo.ID != "debian", !strutil.ListContains(release.ReleaseInfo.IDLike, "debian"):
@mvo5

mvo5 Sep 29, 2017

Collaborator

It seems like an "if" statement here should be fine, a switch {} with just a single case looks a bit out of place.

release/release_test.go
+ c.Check(os.VersionID, Equals, "7")
+ c.Check(os.IDLike, DeepEquals, []string{"rhel", "fedora"})
+}
+
@zyga

zyga Sep 29, 2017

Contributor

I'd like to see a few more test cases for distributions:

  • fedora
  • centos (thanks)
  • rhel
  • ubuntu
  • ubuntu core
  • elementary
  • mint
  • arch

etc.

What we save by having nice generic code here I'd like to check by having very comprehensive test suite that measures behaviour.

@mvo5

mvo5 Sep 29, 2017

Collaborator

AIUI @zyga has a hole bunch of examples of those maybe we can import them here?

@zyga

zyga Sep 29, 2017

Contributor

Yes, I linked to them above.

@Conan-Kudo

Conan-Kudo Sep 29, 2017

Contributor

I don't want to import all of them, as that isn't quite necessary. We just need to prove that the negative cases work.

Conan-Kudo added some commits Sep 28, 2017

@Conan-Kudo
cmd: Adjust re-exec test to test based on distribution family 
By testing on the distribution family, we don't have to keep adding
distribution IDs to disable re-exec where it won't work.

Signed-off-by: Neal Gompa <ngompa13@gmail.com>
5393c23
@Conan-Kudo
dirs: Adjust directory test to test based on distribution family 
By testing on the distribution family, we don't have to keep adding
distribution IDs to deal with the directory path differences.

Signed-off-by: Neal Gompa <ngompa13@gmail.com>

All checks have failed

7 failing checks
@mvo5
artful-amd64 — autopkgtest finished (failure)
Details
@mvo5
artful-i386 — autopkgtest finished (failure)
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
@mvo5
xenial-amd64 — autopkgtest finished (failure)
Details
@mvo5
xenial-i386 — autopkgtest finished (failure)
Details
@mvo5
xenial-ppc64el — autopkgtest finished (failure)
Details
@mvo5
zesty-amd64 — autopkgtest finished (failure)
Details
9a2e010
@zyga

zyga reviewed Sep 29, 2017
View changes

One more comment, I need to check various arch derivatives first.

cmd/cmd.go
@@ -66,8 +66,8 @@ func distroSupportsReExec() bool {
if !release.OnClassic {
return false
}
- switch release.ReleaseInfo.ID {
- case "fedora", "centos", "rhel", "opensuse", "suse", "poky", "arch", "manjaro", "lirios", "solus":
+ if release.ReleaseInfo.ID != "debian" || !strutil.ListContains(release.ReleaseInfo.IDLike, "debian") ||
@zyga

zyga Sep 29, 2017

Contributor

Thank you

dirs/dirs.go
- switch release.ReleaseInfo.ID {
- case "fedora", "centos", "rhel", "arch", "manjaro", "lirios":
+ switch {
+ case release.ReleaseInfo.ID == "fedora", strutil.ListContains(release.ReleaseInfo.IDLike, "fedora"), release.ReleaseInfo.ID == "arch", strutil.ListContains(release.ReleaseInfo.IDLike, "arch"):
@zyga

zyga Sep 29, 2017

Contributor

I'm not sure if Manjaro uses /var/lib/snapd/snap anymore, I think it was reverted back to /snap there

@Conan-Kudo

Conan-Kudo Sep 29, 2017

Contributor

Manjaro doesn't identify as like Arch, so that should be fine.

@Conan-Kudo
Contributor

Conan-Kudo commented Sep 29, 2017

@zyga @mvo5 Help with the tests would be appreciated! I'm at my wit's end with trying to make this pass, and the Wi-Fi is behaving weird now...
@mvo5
add new release.DistroLike() helper and tweak a bit

All checks have passed

7 successful checks
@mvo5
artful-amd64 — autopkgtest finished (success)
Details
@mvo5
artful-i386 — autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
@mvo5
xenial-amd64 — autopkgtest finished (success)
Details
@mvo5
xenial-i386 — autopkgtest finished (success)
Details
@mvo5
xenial-ppc64el — autopkgtest finished (success)
Details
@mvo5
zesty-amd64 — autopkgtest finished (success)
Details
d11d415
@codecov-io

codecov-io commented Sep 30, 2017 •
Edited 1 time

Codecov Report

Merging #3984 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3984      +/-   ##
==========================================
- Coverage   75.94%   75.93%   -0.01%     
==========================================
  Files         423      423              
  Lines       36571    36580       +9     
==========================================
+ Hits        27773    27778       +5     
- Misses       6850     6853       +3     
- Partials     1948     1949       +1
Impacted Files Coverage Δ
dirs/dirs.go 98.11% <100%> (ø) ⬆️
cmd/cmd.go 83.87% <100%> (ø) ⬆️
release/release.go 90.76% <33.33%> (-9.24%) ⬇️
snap/info.go 88.94% <0%> (-0.11%) ⬇️
overlord/snapstate/backend/snapdata.go 56.25% <0%> (+0.61%) ⬆️
overlord/ifacestate/helpers.go 63% <0%> (+0.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6149cd3...d11d415. Read the comment docs.
@Conan-Kudo
Contributor

Conan-Kudo commented Sep 30, 2017

@zyga @mvo5 This is now passing all tests, so can it be merged?

Also, please cherry pick this PR into 2.28, because it'll fix some big issues with Fedora derivatives.
@zyga

zyga approved these changes Oct 2, 2017
View changes

Looks good to me.

@chipaca

chipaca approved these changes Oct 4, 2017
View changes

I think this is fine. FINE.

I'd really like it more if the check were by ability rather than name, but that is Hard.

@mvo5 mvo5 merged commit 5e36fb9 into snapcore:master Oct 4, 2017
7 checks passed

7 checks passed

artful-amd64 autopkgtest finished (success)
Details
artful-i386 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@Conan-Kudo Conan-Kudo deleted the Conan-Kudo:redo-distro-checks branch Oct 4, 2017

@mvo5 mvo5 referenced this pull request Oct 4, 2017

Merged

release,cmd,dirs: Redo the distro checks to take into account distribution families #4001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment