tests,packaging: add package build support for Fedora for our spread setup

[morphis]

This disables all spread tests explicitly for Fedora but keeps a single tests/main/null test active. We use this to introduce just build support inside our spread setup as a first step. As a second step we will add necessary fixes to make all test cases running.

Depends on #3357

[Conan-Kudo]

Please do not merge this with the packaging/fedora-* contents, as I intend to separately provide a PR for packaging contents as soon as we can build and run without patches.

[Conan-Kudo]

Is this a generally needed change? If so, then I should pull this into my snap-mgmt.sh in Fedora Dist-Git.

[morphis]

It is. Without that the snap-mgmt.sh script doesn't work. I would had let you know tomorrow about this as this this prevents our 2.26.x based packages from cleaning up properly. All systemd unit files are prefixed with the snap mount dir which is /var/lib/snapd/snap on Fedora.

[Conan-Kudo]

Please don't do this here. Do this outside of the script, in your invocation script.

[morphis]

Wil do.

[Conan-Kudo]

dnf -q -y remove $package_name

[morphis]

Thanks!

[Conan-Kudo]

dnf -q -y upgrade

[morphis]

Thanks!

[Conan-Kudo]

dnf -q -y autoremove works here, if you need to clean up.

[morphis]

Thanks!

[Conan-Kudo]

addgroup and adduser are Debian specific overlays on top of the common useradd and groupadd commands. The better thing to do here is to migrate to the portable commands entirely.

useradd(8): https://www.mankier.com/8/useradd
groupadd(8): https://www.mankier.com/8/groupadd

[morphis]

That still doesn't help that the useradd tool doesn't support --disable-password or --gecos. However reworked the code a bit to look better and have more in common across different distros.

[Conan-Kudo]

@morphis As an alternative to pulling in the packaging for now, why not just check out the sources from Fedora Dist-Git and use that? That way, you're actually testing how it would be built for Fedora. Not to mention, it allows you to avoid applying a patch that breaks everyone that only applies to certain distros and let you merge this in sooner rather than later.

I'd rather actually check in a copy of the packaging sources into this repo when we're at a point where everything works without any patching...

Packaging sources Git URL: https://src.fedoraproject.org/git/rpms/snapd.git

[morphis]

@morphis As an alternative to pulling in the packaging for now, why not just check out the sources from Fedora Dist-Git and use that? That way, you're actually testing how it would be built for Fedora. Not to mention, it allows you to avoid applying a patch that breaks everyone that only applies to certain distros and let you merge this in sooner rather than later.

This gets tricky as we would have to rebase the patches we have there automatically which doesn't scale nicely and those patches will break quite easily. The opposite way of verifying that the patches we have in Fedora don't break isn't really what we want to verify here. This is intended to ship a clean and patch free version of the Fedora packaging.

I'd rather actually check in a copy of the packaging sources into this repo when we're at a point where everything works without any patching...

The idea is to land this and the Fedora packaging we verify here should work without any patch. We're already close as there is only a single one left (as pointed out in the description).

If you want to provide a PR with a single patch (for snap-confine) + the fedora packaging, please do this now. We will fix the last patch soon but I don't want to wait with the spread test setup much longer.

[Conan-Kudo]

@morphis Done, see #3366.

[zyga]

Some nitpicks, it would be good to document why tests are disabled for Fedora (even with a simple TODO: enable this for Fedora later) because this has the chance of getting stale over time and nobody will remember why.

[zyga]

You could do dnf info snapd or something similar here.

[morphis]

Fixed

[zyga]

Ditto

[morphis]

Fixed

[morphis]

@zyga Added a comment for all disabled test cases.

[niemeyer]

Please let me have a look before merging.

[Conan-Kudo]

This is not a good idea.. This is not meant to be run by humans, only tools. That's explicitly why it's in %_libexecdir/snapd rather than being in %_bindir.

[morphis]

Actually I don't see a difference if the script is called by a human or just another script. That it is not intended to be called by humans is already encoded by putting it no into %_bindir as you say. Providing a help output and proper command line options makes it just more convenient for developers to know what a script they call from another one does instead of relying on non obvious positional command line options. Hiding these things doesn't help in any case.

[niemeyer]

Yeah, I can't find it bad to have actual documentation. Even if called from another tool, it'll be a human touching both ends.

[Conan-Kudo]

Again, not particularly a fan of this change.

[Conan-Kudo]

If you want to force metadata update (like you do for deb systems), you can tack on --refresh.

[morphis]

Thanks, will add!

[codecov-io]

Codecov Report

Merging #3365 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3365   +/-   ##
=======================================
  Coverage   77.24%   77.24%           
=======================================
  Files         371      371           
  Lines       25519    25519           
=======================================
  Hits        19711    19711           
+ Misses       4058     4057    -1     
- Partials     1750     1751    +1

Impacted Files	Coverage Δ
cmd/snap/cmd_aliases.go	94% <0%> (-2%)	⬇️
overlord/snapstate/snapstate.go	81.26% <0%> (-0.24%)	⬇️
httputil/retry.go	93.22% <0%> (+5.08%)	⬆️

---

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 556642c...7b80258. Read the comment docs.

[fgimenez]

Looks good thanks! Some comments inline, most important IMO about the system restrictions

[fgimenez]

Maybe a simple if in this case, given that there's only one branch?

[niemeyer]

The case matching probably looks more clear than the if version will look like.

[fgimenez]

I think this is not needed given that the system has been disabled at the main suite level in https://github.com/snapcore/snapd/pull/3365/files#diff-3c11e56e5f7f82b0f352d0fe1851a107R306. There are lots of systems entries for tasks in main, not sure if they or the entry at the suite level should be removed, but we shouldn't have both.

[niemeyer]

Indeed, and same for all the cases below. It also means we don't need all those comments, which is nice. Simply enable for fedora one-by-one, until we finally don't need the disabling at the suite level anymore and can take all of them off.

[morphis]

That the tests/main suite is disabled is actually a mistake. My bad that I tested locally only with an explicit linode:fedora-25-64:tests/main/null. However, we can't disable all test cases in the beginning for Fedora as we need at least one for spread to prepare the system. Otherwise it will simply find no matches for the system and don't bother with it at all. That is why I added a tests/main/null test case and disabled all others.

Also a disabled tests/main suite with a single sub test enables still gives

$ spread -v -debug -reuse -resend linode:fedora-25-64:tests/main
2017/05/30 07:05:34 Found /home/simon/Work/ubuntu/snappy/work/src/github.com/snapcore/snapd/spread.yaml.
error: nothing matches provider filter

[fgimenez]

Couldn't we use one of the existing tests for this? The test/main/help variants are very simple, as soon as snap is available from the command line they should work.

[niemeyer]

Indeed.. if we can't pass any test at all, it means the suite isn't working anyway. So we may as well just not have anything.

[morphis]

I can change this to tests/main/help. Having nothing is not an option as explained above.

[niemeyer]

Let's please start with 1 here.

[morphis]

Ok.

[niemeyer]

As someone sitting outside your plans, this doesn't make much sense. There's no hint about what that's supposed to be, or why it would help here. The fact it doesn't even exist makes it slightly hard to find out as well. :)

Either the comment needs to be improved, or dropped.

[morphis]

I was already asked once or twice why I don't use tests/lib/pkgdb.sh here. So I will leave the comment to make that visible but improve it a bit.

[niemeyer]

Is that necessary? What's the default behavior for the case statement?

[niemeyer]

Can we please drop both of these comments? They're very likely wrong already.

[morphis]

Done.

[niemeyer]

Wonder why inverted these function names are...

[zyga]

You must unlearn what you have learned. ;-)

[niemeyer]

I think we can call this as build_rpm and in the future just have the deltas internally, instead of having completely different instructions for each distro.

[morphis]

That isn't that easy. The build process looks different on Fedora and openSUSE. On Fedora we really want to utilize mock to ease as much as possible of the actual package build but on openSUSE we have to stick with the plain old rpm command. We can write a super generic part of this but it might get too generic in the end with all the different corner cases (vendoring vs. no vendoring, mock vs. rpm, package build location, ...). I tend to start with a specialized version of both and see where we end up and then refactor if there is a need to.

[Conan-Kudo]

If we want to, we could use rpmbuild for both distros. We'd need to use dnf builddep for Fedora to install the builddeps and whatnot, but it's doable. It gets tricky due to the switches we flip, though.

[morphis]

Yes, we can strip out a generic portion. I would like to get specific fedora_build_rpm and opensuse_build_rpm functions in for now as this is already waiting for too long. There will be a follow up PR to combine as much as possible.

[morphis]

I have a more generic build_rpm now. Lets land this as is and I will propose a PR once the suse/fedora ones landed to convert both into a single build_rpm function. @niemeyer @Conan-Kudo OK for you guys?

[niemeyer]

Again, we don't need that if it's a passthrough I think.

That said, shouldn't this be an error in this case?

[morphis]

Done.

[fgimenez]

Maybe a simple if in this case, given that there's only one branch?

[niemeyer]

The case matching probably looks more clear than the if version will look like.

[niemeyer]

We probably don't need this line though.

[niemeyer]

Same.

[niemeyer]

Same.

[niemeyer]

Same.

[niemeyer]

I think this can be just:

$GRUB_EDITNV list | MATCH -v snap_

[niemeyer]

The -q 1 was already in use even without Fedora before. It's strange that by adding Fedora we're adding custom code that adds the flag just inside Fedora, and disables it elsewhere where it was already working. Seems like the new logic is unnecessary?

[morphis]

The logic is the other way round. It drops -q 1 only on Fedora as it ships with a different netcat implementation (not the -openbsd variant as Debian/Ubuntu do) which does not support -q.

[Conan-Kudo]

Fedora offers the netcat implementation from the nmap project. If Ubuntu also offers it, you could harmonize on that.

[morphis]

Ubuntu only has the -openbsd variant and the traditional one from http://www.stearns.org/nc/

[Conan-Kudo]

It's /usr/bin/ncat everywhere (including Ubuntu, where it ships as the nmap package). /usr/bin/nc is symlinked in Fedora to ncat.

[morphis]

Ah its coming via the nmap package on Ubuntu (https://packages.ubuntu.com/yakkety/nmap). That is why I didn't found it. Let me look into replacement nc here with ncat in another PR as there are more places than this one suffering from the same problem.

[morphis]

Ok, I have an replacement for this coming with another PR. Would prefer to keep this as is and fix it properly with a followup PR.

[fgimenez]

I think this is not needed given that the system has been disabled at the main suite level in https://github.com/snapcore/snapd/pull/3365/files#diff-3c11e56e5f7f82b0f352d0fe1851a107R306. There are lots of systems entries for tasks in main, not sure if they or the entry at the suite level should be removed, but we shouldn't have both.

[niemeyer]

Indeed, and same for all the cases below. It also means we don't need all those comments, which is nice. Simply enable for fedora one-by-one, until we finally don't need the disabling at the suite level anymore and can take all of them off.

[morphis]

That the tests/main suite is disabled is actually a mistake. My bad that I tested locally only with an explicit linode:fedora-25-64:tests/main/null. However, we can't disable all test cases in the beginning for Fedora as we need at least one for spread to prepare the system. Otherwise it will simply find no matches for the system and don't bother with it at all. That is why I added a tests/main/null test case and disabled all others.

Also a disabled tests/main suite with a single sub test enables still gives

$ spread -v -debug -reuse -resend linode:fedora-25-64:tests/main
2017/05/30 07:05:34 Found /home/simon/Work/ubuntu/snappy/work/src/github.com/snapcore/snapd/spread.yaml.
error: nothing matches provider filter

[fgimenez]

Couldn't we use one of the existing tests for this? The test/main/help variants are very simple, as soon as snap is available from the command line they should work.

[niemeyer]

Indeed.. if we can't pass any test at all, it means the suite isn't working anyway. So we may as well just not have anything.

[morphis]

I can change this to tests/main/help. Having nothing is not an option as explained above.

[Conan-Kudo]

What? This isn't right. It's python3-PyYAML. Also, python3-yaml name is provided by the python3-PyYAML package.

[morphis]

Fixed. Not sure why I used python3-yamlordereddictloader instead. Let's see what fails.

[Conan-Kudo]

What is this version construct supposed to be? Why not just do $basever+git${commitdate}.${shorthash} or something more useful?

[morphis]

I am following the same scheme as the debian build does here. I guess 1337 is set to have a high enough version so that it overrides any other possibly installed versions. @mvo5 might know more. Just for the sake of the build and nothing which will ever end in the tree.

[mvo5]

Yes, this version is picked to ensure its absurdly high to ensure we always have something more current installed than what is in the core snap.

[Conan-Kudo]

@mvo5 Do Epochs not work the same way in dpkg as they do in rpm?

[niemeyer]

This is just for testing.. it's actually nice to have something so obviously unreasonable in that context. Makes it clear this is not just a normal build.

[morphis]

@niemeyer Switched to a whitelisting approach now. Please have another look.

[niemeyer]

Thanks for changing into a whitelist. LGTM.

Tests are broken, though, and the error seems real:

+ rm -rf squashfs-root
+ /usr/lib/snapd/snap-discard-ns core
/home/gopath/src/github.com/snapcore/snapd/tests/lib/prepare.sh: line 65: /usr/lib/snapd/snap-discard-ns: No such file or directory

[morphis]

@niemeyer Thanks. Fixed the failing call.

[zyga]

LGTM, comments some for consideration yours

[niemeyer]

Wonder why inverted these function names are...

[zyga]

You must unlearn what you have learned. ;-)

[zyga]

For the purpose of writing tests I was thinking about snap tool discard-ns core that could aggregate all distro differences this way.