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

Message ID cu7y38han6g.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/applying patch fail Apply failed

Commit Message

Arun Isaac Dec. 22, 2018, 10:08 a.m. UTC
Please find attached an updated patch.

Comments

Ludovic Courtès Dec. 23, 2018, 5:23 p.m. UTC | #1
Hi Arun,

Arun Isaac <arunisaac@systemreboot.net> skribis:

> From de88021c9a73d28f11bc2e060098484bd414da62 Mon Sep 17 00:00:00 2001
> From: Arun Isaac <arunisaac@systemreboot.net>
> Date: Fri, 21 Dec 2018 17:48:55 +0530
> Subject: [PATCH] guix: lint: Check for source URIs redirecting to GitHub.
>
> * guix/scripts/lint.scm (check-github-uri): New procedure.
> (%checkers): Add it.
> * doc/guix.texi (Invoking guix lint): Document it.
> * tests/lint.scm ("github-url", "github-url: one suggestion"): New tests.

LGTM, thank you!

Ludo’.
Ludovic Courtès Jan. 5, 2019, 11:18 p.m. UTC | #2
Hello,

Arun Isaac <arunisaac@systemreboot.net> skribis:

>>From de88021c9a73d28f11bc2e060098484bd414da62 Mon Sep 17 00:00:00 2001
> From: Arun Isaac <arunisaac@systemreboot.net>
> Date: Fri, 21 Dec 2018 17:48:55 +0530
> Subject: [PATCH] guix: lint: Check for source URIs redirecting to GitHub.
>
> * guix/scripts/lint.scm (check-github-uri): New procedure.
> (%checkers): Add it.
> * doc/guix.texi (Invoking guix lint): Document it.
> * tests/lint.scm ("github-url", "github-url: one suggestion"): New tests.

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---

I think that’s because the above URL redirects to
<https://github-production-release-asset-2e65be.s3.amazonaws.com/18719856/7aec3148-7d35-11e8-9ad9-0a369f8ccc41?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20190105%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20190105T231113Z&X-Amz-Expires=300&X-Amz-Signature=fc2594a6b3dbfd2e841317adaec1ca71482bf20f2c53e8eb7e7c343f950f74ce&X-Amz-SignedHeaders=host&actor_id=0&response-content-disposition=attachment%3B%20filename%3Dstellarium-0.18.1.tar.gz&response-content-type=application%2Foctet-stream>.

Any idea how we could avoid that?

Thanks,
Ludo’.

Patch

From de88021c9a73d28f11bc2e060098484bd414da62 Mon Sep 17 00:00:00 2001
From: Arun Isaac <arunisaac@systemreboot.net>
Date: Fri, 21 Dec 2018 17:48:55 +0530
Subject: [PATCH] guix: lint: Check for source URIs redirecting to GitHub.

* guix/scripts/lint.scm (check-github-uri): New procedure.
(%checkers): Add it.
* doc/guix.texi (Invoking guix lint): Document it.
* tests/lint.scm ("github-url", "github-url: one suggestion"): New tests.
---
 doc/guix.texi         | 10 ++++++----
 guix/scripts/lint.scm | 39 +++++++++++++++++++++++++++++++++++++++
 tests/lint.scm        | 28 ++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 8f6a8b3ed..62e0454cc 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -7659,12 +7659,14 @@  Identify inputs that should most likely be native inputs.
 @item source
 @itemx home-page
 @itemx mirror-url
+@itemx github-url
 @itemx source-file-name
 Probe @code{home-page} and @code{source} URLs and report those that are
-invalid.  Suggest a @code{mirror://} URL when applicable.  Check that
-the source file name is meaningful, e.g.@: is not
-just a version number or ``git-checkout'', without a declared
-@code{file-name} (@pxref{origin Reference}).
+invalid.  Suggest a @code{mirror://} URL when applicable.  If the
+@code{source} URL redirects to a GitHub URL, recommend usage of the GitHub
+URL.  Check that the source file name is meaningful, e.g.@: is not just a
+version number or ``git-checkout'', without a declared @code{file-name}
+(@pxref{origin Reference}).
 
 @item cve
 @cindex security vulnerabilities
diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 2314f3b28..354f6f703 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -8,6 +8,7 @@ 
 ;;; Copyright © 2017 Alex Kost <alezost@gmail.com>
 ;;; Copyright © 2017 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
+;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -44,8 +45,10 @@ 
   #:use-module (guix cve)
   #:use-module (gnu packages)
   #:use-module (ice-9 match)
+  #:use-module (ice-9 receive)
   #:use-module (ice-9 regex)
   #:use-module (ice-9 format)
+  #:use-module (web client)
   #:use-module (web uri)
   #:use-module ((guix build download)
                 #:select (maybe-expand-mirrors
@@ -74,6 +77,7 @@ 
             check-source
             check-source-file-name
             check-mirror-url
+            check-github-url
             check-license
             check-vulnerabilities
             check-for-updates
@@ -773,6 +777,37 @@  descriptions maintained upstream."
       (let ((uris (origin-uris origin)))
         (for-each check-mirror-uri uris)))))
 
+(define (check-github-url package)
+  "Check whether PACKAGE uses source URLs that redirect to GitHub."
+  (define (follow-redirect uri)
+    (receive (response body) (http-head uri)
+      (case (response-code response)
+        ((301 302)
+         (uri->string (assoc-ref (response-headers response) 'location)))
+        (else #f))))
+
+  (define (follow-redirects-to-github uri)
+    (cond
+     ((string-prefix? "https://github.com/" uri) uri)
+     ((string-prefix? "http" uri)
+      (and=> (follow-redirect uri) follow-redirects-to-github))
+     ;; Do not attempt to follow redirects on URIs other than http and https
+     ;; (such as mirror, file)
+     (else #f)))
+
+  (let ((origin (package-source package)))
+    (when (and (origin? origin)
+               (eqv? (origin-method origin) url-fetch))
+      (for-each
+       (lambda (uri)
+         (and=> (follow-redirects-to-github uri)
+                (lambda (github-uri)
+                  (emit-warning
+                   package
+                   (format #f (G_ "URL should be '~a'") github-uri)
+                   'source))))
+       (origin-uris origin)))))
+
 (define (check-derivation package)
   "Emit a warning if we fail to compile PACKAGE to a derivation."
   (define (try system)
@@ -1055,6 +1090,10 @@  or a list thereof")
      (name        'mirror-url)
      (description "Suggest 'mirror://' URLs")
      (check       check-mirror-url))
+   (lint-checker
+     (name        'github-uri)
+     (description "Suggest GitHub URIs")
+     (check       check-github-url))
    (lint-checker
      (name        'source-file-name)
      (description "Validate file names of sources")
diff --git a/tests/lint.scm b/tests/lint.scm
index 300153e24..d4aa7c0e8 100644
--- a/tests/lint.scm
+++ b/tests/lint.scm
@@ -6,6 +6,7 @@ 
 ;;; Copyright © 2016 Hartmut Goebel <h.goebel@crazy-compilers.com>
 ;;; Copyright © 2017 Alex Kost <alezost@gmail.com>
 ;;; Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
+;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -669,6 +670,33 @@ 
        (check-mirror-url (dummy-package "x" (source source)))))
    "mirror://gnu/foo/foo.tar.gz"))
 
+(test-assert "github-url"
+  (string-null?
+   (with-warnings
+     (with-http-server 200 %long-string
+       (check-github-url
+        (dummy-package "x" (source
+                            (origin
+                              (method url-fetch)
+                              (uri (%local-url))
+                              (sha256 %null-sha256)))))))))
+
+(let ((github-url "https://github.com/foo/bar/bar-1.0.tar.gz"))
+  (test-assert "github-url: one suggestion"
+    (string-contains
+     (with-warnings
+       (with-http-server (301 `((location . ,(string->uri github-url)))) ""
+         (let ((initial-uri (%local-url)))
+           (parameterize ((%http-server-port (+ 1 (%http-server-port))))
+             (with-http-server (302 `((location . ,(string->uri initial-uri)))) ""
+               (check-github-url
+                (dummy-package "x" (source
+                                    (origin
+                                      (method url-fetch)
+                                      (uri (%local-url))
+                                      (sha256 %null-sha256))))))))))
+     github-url)))
+
 (test-assert "cve"
   (mock ((guix scripts lint) package-vulnerabilities (const '()))
         (string-null?
-- 
2.19.2