Merge ~xnox/review-tools:riscv64 into review-tools:master

[Dimitri John Ledkov]

Add riscv64 architecture.

[Jamie Strandboge (jdstrand)]

LGTM. Thanks! Note, other changes are needed to make a riscv64 snap of the review-tools, but that is a separate issue.

review: Approve

[Dimitri John Ledkov (xnox)]

> LGTM. Thanks! Note, other changes are needed to make a riscv64 snap of the
> review-tools, but that is a separate issue.

Do review-tools in production run on the matching architecture for the snap under review?
If so, this complicates bootstrap of the snaps that I did not predict.

At the moment, I am bootstraping by hand: snapd snapcraft core20 lxd. Cause all of them are snaps, and need each other to snap them. Is review-tools needed to be added to this mix?

Or will I be allowed to upload snapd/snapcraft/core20/lxd to the store for the riscv64 architecture without review-tools riscv64 snap?

The set of by hand bootstrapped snaps is the list required for launchpad to be able to build snaps and enable snap builders.

[Daniel Manrique (roadmr)]

On Tue, Jun 23, 2020 at 4:00 PM Dimitri John Ledkov <email address hidden>
wrote:

> > LGTM. Thanks! Note, other changes are needed to make a riscv64 snap of
> the
> > review-tools, but that is a separate issue.
>
> Do review-tools in production run on the matching architecture for the
> snap under review?
> If so, this complicates bootstrap of the snaps that I did not predict.
>

They do not. Review-tools in production run on amd64 regardless of the
snaps' target architecture.

You will need an arch-specific build if you want to run review-tools to
pre-check your snaps before uploading. I believe snapcraft will try to do
this for you optionally. But AFAIK if you don't have review-tools installed
snapcraft will not barf, it'll just say you could be doing better :)

>
> At the moment, I am bootstraping by hand: snapd snapcraft core20 lxd.
> Cause all of them are snaps, and need each other to snap them. Is
> review-tools needed to be added to this mix?
>
> Or will I be allowed to upload snapd/snapcraft/core20/lxd to the store for
> the riscv64 architecture without review-tools riscv64 snap?
>
> The set of by hand bootstrapped snaps is the list required for launchpad
> to be able to build snaps and enable snap builders.
> --
>
> https://code.launchpad.net/~xnox/review-tools/+git/review-tools/+merge/386272
> Your team MyApps reviewers is requested to review the proposed merge of
> ~xnox/review-tools:riscv64 into review-tools:master.
>

--
- Daniel

[Jamie Strandboge (jdstrand)]

Right, a riscv64 review-tools snap is not required for any store reviews or snapcraft builds. I'm not sure of the UX for snapcraft in suggesting to install the review-tools when it isn't yet available for the arch, but that isn't anything for this PR. I mostly wanted to point out that this only enables store reviews and not snapcraft's running of the tools.

[Jamie Strandboge (jdstrand)]

This is merged and ready to pull into prod via the 20200623-2050UTC tag.

diff --git a/reviewtools/sr_common.py b/reviewtools/sr_common.py
index f5c0c23..342783b 100644
--- a/reviewtools/sr_common.py
+++ b/reviewtools/sr_common.py
@@ -180,12 +180,13 @@ class SnapReview(Review):
     valid_system_usernames = ["snap_daemon"]
 
     valid_compiled_architectures = [
-        "armhf",
-        "i386",
         "amd64",
         "arm64",
+        "armhf",
+        "i386",
         "powerpc",
         "ppc64el",
+        "riscv64",
         "s390x",
     ]
 
diff --git a/reviewtools/tests/test_sr_lint.py b/reviewtools/tests/test_sr_lint.py
index b4559f0..c86e44e 100644
--- a/reviewtools/tests/test_sr_lint.py
+++ b/reviewtools/tests/test_sr_lint.py
@@ -804,6 +804,15 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
         expected_counts = {"info": 1, "warn": 0, "error": 0}
         self.check_results(r, expected_counts)
 
+    def test_check_architectures_single_riscv64(self):
+        """Test check_architectures() (single arch, riscv64)"""
+        self.set_test_snap_yaml("architectures", ["riscv64"])
+        c = SnapReviewLint(self.test_name)
+        c.check_architectures()
+        r = c.review_report
+        expected_counts = {"info": 1, "warn": 0, "error": 0}
+        self.check_results(r, expected_counts)
+
     def test_check_architectures_single_s390x(self):
         """Test check_architectures() (single arch, s390x)"""
         self.set_test_snap_yaml("architectures", ["s390x"])