diff mbox series

[bug#33801] import: github: Support source URIs that redirect to GitHub

Message ID cu78szw8ilg.fsf@systemreboot.net
State Accepted
Headers show
Series [bug#33801] import: github: Support source URIs that redirect to GitHub | expand

Checks

Context Check Description
cbaines/package builds pending In Cuirass
cbaines/applying patch fail Apply failed

Commit Message

Arun Isaac Jan. 7, 2019, 5:48 p.m. UTC 
> I just realized that the warning also triggers when the URL is already a
> github.com URL:

> --8<---------------cut here---------------start------------->8---
> $ ./pre-inst-env guix lint -c github-uri stellarium
> gnu/packages/astronomy.scm:135:12: stellarium@0.18.1: URL should be 'https://github.com/Stellarium/stellarium/releases/download/v0.18.1/stellarium-0.18.1.tar.gz'
> --8<---------------cut here---------------end--------------->8---

This is a simple mistake on my part and it's not because stellarium's
origin URI redirects to the github-production-release-asset URI. Please
find attached a patch addressing this.

Comments

Ludovic Courtès Jan. 8, 2019, 8:40 a.m. UTC | #1 
Hello,

Arun Isaac <arunisaac@systemreboot.net> skribis:

> This is a simple mistake on my part and it's not because stellarium's
> origin URI redirects to the github-production-release-asset URI. Please
> find attached a patch addressing this.
>
> From a8bc92f507e35b54afdd24a42f0aa45ff7cb2c0b Mon Sep 17 00:00:00 2001
> From: Arun Isaac <arunisaac@systemreboot.net>
> Date: Mon, 7 Jan 2019 23:11:58 +0530
> Subject: [PATCH] guix: lint: Warn only if GitHub URI is not same as the
>  package URI.
>
> * guix/scripts/lint.scm (check-github-url): Warn only if the GitHub URI
> obtained after following redirects is not same as the original URI.

Oh, I see.  Perhaps we should add a test to catch this specific case so
that it doesn’t pop up again?

Otherwise LGTM.

Thanks,
Ludo’.
Arun Isaac Jan. 8, 2019, 1:19 p.m. UTC | #2 
> Perhaps we should add a test to catch this specific case so that it
> doesn’t pop up again?

Yes, we should. But, there is a problem. "make check
TESTS=tests/lint.scm" fails with the following error message even on
master.

make[4]: *** [Makefile:4920: tests/lint.log] Error 1
make[4]: Leaving directory '/home/foo/guix'
make[3]: *** [Makefile:4902: check-TESTS] Error 2
make[3]: Leaving directory '/home/foo/guix'
make[2]: *** [Makefile:5145: check-am] Error 2
make[2]: Leaving directory '/home/foo/guix'
make[1]: *** [Makefile:4679: check-recursive] Error 1
make[1]: Leaving directory '/home/foo/guix'
make: *** [Makefile:5147: check] Error 2

On examining tests/lint.log, I find the following error message.

gnu/packages/lisp.scm:3806:7: In procedure inputs:
error: xclip: unbound variable

But, this makes no sense to me. Any idea what's happening?

Should I immediately push the patch I sent in the last mail, and then
deal with this problem separately? Without that patch, the linter is
faulty and might annoy people.
Ludovic Courtès Jan. 9, 2019, 2:11 p.m. UTC | #3 
Hello Arun,

Arun Isaac <arunisaac@systemreboot.net> skribis:

>> Perhaps we should add a test to catch this specific case so that it
>> doesn’t pop up again?
>
> Yes, we should. But, there is a problem. "make check
> TESTS=tests/lint.scm" fails with the following error message even on
> master.
>
> make[4]: *** [Makefile:4920: tests/lint.log] Error 1
> make[4]: Leaving directory '/home/foo/guix'
> make[3]: *** [Makefile:4902: check-TESTS] Error 2
> make[3]: Leaving directory '/home/foo/guix'
> make[2]: *** [Makefile:5145: check-am] Error 2
> make[2]: Leaving directory '/home/foo/guix'
> make[1]: *** [Makefile:4679: check-recursive] Error 1
> make[1]: Leaving directory '/home/foo/guix'
> make: *** [Makefile:5147: check] Error 2
>
> On examining tests/lint.log, I find the following error message.
>
> gnu/packages/lisp.scm:3806:7: In procedure inputs:
> error: xclip: unbound variable
>
> But, this makes no sense to me. Any idea what's happening?

Ouch.  This is fixed by 804b9b18ac9188ffb6c6891cbb9241c6a80ed7c8.  I
think we were just lucky it didn’t bite before.

> Should I immediately push the patch I sent in the last mail, and then
> deal with this problem separately? Without that patch, the linter is
> faulty and might annoy people.

You can recheck your patch and push it I guess.

Thanks,
Ludo’.
diff mbox series

Patch

From a8bc92f507e35b54afdd24a42f0aa45ff7cb2c0b Mon Sep 17 00:00:00 2001
From: Arun Isaac <arunisaac@systemreboot.net>
Date: Mon, 7 Jan 2019 23:11:58 +0530
Subject: [PATCH] guix: lint: Warn only if GitHub URI is not same as the
 package URI.

* guix/scripts/lint.scm (check-github-url): Warn only if the GitHub URI
obtained after following redirects is not same as the original URI.
---
 guix/scripts/lint.scm | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 9acec4857..0f315a935 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -8,7 +8,7 @@ 
 ;;; Copyright © 2017 Alex Kost <alezost@gmail.com>
 ;;; Copyright © 2017 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2017, 2018 Efraim Flashner <efraim@flashner.co.il>
-;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
+;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -820,10 +820,11 @@  descriptions maintained upstream."
        (lambda (uri)
          (and=> (follow-redirects-to-github uri)
                 (lambda (github-uri)
-                  (emit-warning
-                   package
-                   (format #f (G_ "URL should be '~a'") github-uri)
-                   'source))))
+                  (unless (string=? github-uri uri)
+                    (emit-warning
+                     package
+                     (format #f (G_ "URL should be '~a'") github-uri)
+                     'source)))))
        (origin-uris origin)))))
 
 (define (check-derivation package)
-- 
2.19.2