[bug#38460] [PATCH 0/1] lint: Add '--load-path' option.

[bug#38460] [PATCH 0/1] lint: Add '--load-path' option.

[zimoun]

Dear,

Currently, when developing personal packages, "guix lint" needs the variable GUIX_PACKAGE_PATH which is not user friendly, IMO. The patch uses the common option '-L' to prepend a load path.

Other said,

  GUIX_PACKAGE_PATH=/path/to/my/package guix lint foo

becomes

  guix lint -L /path/to/my/package foo


All the best,
simon


zimoun (1):
  lint: Add '--load-path' option.

 guix/scripts/lint.scm |  8 ++++++++
 tests/guix-lint.sh    | 12 ++++++++++++
 2 files changed, 20 insertions(+)

-- 
2.23.0

[bug#38460] [PATCH 1/1] lint: Add '--load-path' option.

[zimoun]

* guix/scripts/lint.scm (%options): Add '--load-path' option.
* tests/guix-lint.sh: Test it.
---
 guix/scripts/lint.scm |  8 ++++++++
 tests/guix-lint.sh    | 12 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 1668d02992..8d08c484f5 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -9,6 +9,7 @@
 ;;; Copyright © 2017 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2017, 2018 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net>
+;;; Copyright © 2019 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -30,6 +31,7 @@
   #:use-module (guix lint)
   #:use-module (guix ui)
   #:use-module (guix scripts)
+  #:use-module (guix scripts build)
   #:use-module (gnu packages)
   #:use-module (ice-9 match)
   #:use-module (ice-9 format)
@@ -94,6 +96,9 @@ run the checkers on all packages.\n"))
   -c, --checkers=CHECKER1,CHECKER2...
                          only run the specified checkers"))
   (display (G_ "
+  -L, --load-path=DIR    prepend DIR to the package module search path"))
+  (newline)
+  (display (G_ "
   -h, --help             display this help and exit"))
   (display (G_ "
   -l, --list-checkers    display the list of available lint checkers"))
@@ -128,6 +133,9 @@ run the checkers on all packages.\n"))
                               %local-checkers
                               (alist-delete 'checkers
                                             result))))
+        (find (lambda (option)
+                (member "load-path" (option-names option)))
+              %standard-build-options)
         (option '(#\h "help") #f #f
                 (lambda args
                   (show-help)
diff --git a/tests/guix-lint.sh b/tests/guix-lint.sh
index 7ddc7c265b..f07ccb4e1a 100644
--- a/tests/guix-lint.sh
+++ b/tests/guix-lint.sh
@@ -76,3 +76,15 @@ then true; else false; fi
 
 # Make sure specifying multiple packages works.
 guix lint -c inputs-should-be-native dummy dummy@42 dummy
+
+
+# Use --load-path instead.
+unset GUIX_PACKAGE_PATH
+LOAD_PATH="$module_dir"
+
+out=`guix lint -L $LOAD_PATH -c synopsis,description dummy 2>&1`
+if [ `grep_warning "$out"` -ne 3 ]
+then false; else true; fi
+
+# Make sure specifying multiple packages works.
+guix lint -L $LOAD_PATH -c inputs-should-be-native dummy dummy@42 dummy
-- 
2.23.0

[bug#38460] [PATCH 1/1] lint: Add '--load-path' option.

[Ludovic Courtès]

Hi!

zimoun <zimon.toutoune@gmail.com> skribis:

> * guix/scripts/lint.scm (%options): Add '--load-path' option.
> * tests/guix-lint.sh: Test it.

Awesome, I’ve been wanting that for a long time.  :-)

> +LOAD_PATH="$module_dir"
> +
> +out=`guix lint -L $LOAD_PATH -c synopsis,description dummy 2>&1`

Perhaps you don’t even need to define ‘LOAD_PATH’?

Could you add ‘-L’ to guix.texi, and then I guess we’re done!

Thanks,
Ludo’.

[bug#38460] [PATCH v2] lint: Add '--load-path' option.

[zimoun]

* guix/scripts/lint.scm (%options): Add '--load-path' option.
* doc/guix.texi: Document it.
* tests/guix-lint.sh: Test it.
---
 doc/guix.texi         |  8 ++++++++
 guix/scripts/lint.scm |  8 ++++++++
 tests/guix-lint.sh    | 11 +++++++++++
 3 files changed, 27 insertions(+)

diff --git a/doc/guix.texi b/doc/guix.texi
index 2da1ecd64c..e20d3fa722 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -9676,6 +9676,14 @@ and exit.
 Only enable the checkers specified in a comma-separated list using the
 names returned by @code{--list-checkers}.
 
+@item --load-path=@var{directory}
+@itemx -L @var{directory}
+Add @var{directory} to the front of the package module search path
+(@pxref{Package Modules}).
+
+This allows users to define their own packages and make them visible to
+the command-line tools.
+
 @end table
 
 @node Invoking guix size
diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 1668d02992..8d08c484f5 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -9,6 +9,7 @@
 ;;; Copyright © 2017 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2017, 2018 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net>
+;;; Copyright © 2019 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -30,6 +31,7 @@
   #:use-module (guix lint)
   #:use-module (guix ui)
   #:use-module (guix scripts)
+  #:use-module (guix scripts build)
   #:use-module (gnu packages)
   #:use-module (ice-9 match)
   #:use-module (ice-9 format)
@@ -94,6 +96,9 @@ run the checkers on all packages.\n"))
   -c, --checkers=CHECKER1,CHECKER2...
                          only run the specified checkers"))
   (display (G_ "
+  -L, --load-path=DIR    prepend DIR to the package module search path"))
+  (newline)
+  (display (G_ "
   -h, --help             display this help and exit"))
   (display (G_ "
   -l, --list-checkers    display the list of available lint checkers"))
@@ -128,6 +133,9 @@ run the checkers on all packages.\n"))
                               %local-checkers
                               (alist-delete 'checkers
                                             result))))
+        (find (lambda (option)
+                (member "load-path" (option-names option)))
+              %standard-build-options)
         (option '(#\h "help") #f #f
                 (lambda args
                   (show-help)
diff --git a/tests/guix-lint.sh b/tests/guix-lint.sh
index 7ddc7c265b..f0df1fda3a 100644
--- a/tests/guix-lint.sh
+++ b/tests/guix-lint.sh
@@ -76,3 +76,14 @@ then true; else false; fi
 
 # Make sure specifying multiple packages works.
 guix lint -c inputs-should-be-native dummy dummy@42 dummy
+
+
+# Use --load-path instead.
+unset GUIX_PACKAGE_PATH
+
+out=`guix lint -L $module_dir -c synopsis,description dummy 2>&1`
+if [ `grep_warning "$out"` -ne 3 ]
+then false; else true; fi
+
+# Make sure specifying multiple packages works.
+guix lint -L $module_dir -c inputs-should-be-native dummy dummy@42 dummy
-- 
2.23.0

[bug#38460] [PATCH 1/1] lint: Add '--load-path' option.

[zimoun]

Hi,

On Wed, 4 Dec 2019 at 18:10, Ludovic Courtès <ludo@gnu.org> wrote:

> zimoun <zimon.toutoune@gmail.com> skribis:
>
> > * guix/scripts/lint.scm (%options): Add '--load-path' option.
> > * tests/guix-lint.sh: Test it.
>
> Awesome, I’ve been wanting that for a long time.  :-)

But I find this "hacky". Because it is copy/paste from other
--load-path option elsewhere (probably "guix show" :-)).
It should be refactored to have only one function and call it where
the command needs it.
I mean, the same way that 'show-build-options-help' or '(append
%transformation-options %standard-build-options)'  does in "guix
package".
Other said, %standard-build-options should be splited and go to say
misc.scm or common.scm or whatever -- I do not know enough the file
organization. :-)

Now, the same --load-path code at 3 different places. Bad practise... IMHO.

Plus, we will win more flexibility to write more commands. ;-)

What do you think?



> > +LOAD_PATH="$module_dir"
> > +
> > +out=`guix lint -L $LOAD_PATH -c synopsis,description dummy 2>&1`
>
> Perhaps you don’t even need to define ‘LOAD_PATH’?

Done.


> Could you add ‘-L’ to guix.texi, and then I guess we’re done!

I did.
My bad.


Cheers,
simon

[bug#38460] [PATCH 1/1] lint: Add '--load-path' option.

[Ludovic Courtès]

Hi!

zimoun <zimon.toutoune@gmail.com> skribis:

> On Wed, 4 Dec 2019 at 18:10, Ludovic Courtès <ludo@gnu.org> wrote:
>
>> zimoun <zimon.toutoune@gmail.com> skribis:
>>
>> > * guix/scripts/lint.scm (%options): Add '--load-path' option.
>> > * tests/guix-lint.sh: Test it.
>>
>> Awesome, I’ve been wanting that for a long time.  :-)
>
> But I find this "hacky". Because it is copy/paste from other
> --load-path option elsewhere (probably "guix show" :-)).

Well, technically speaking it’s not copy/pasted since the code you sent
uses ‘find’ to grab the option; plus, we’re talking about very few
lines, which is typically hard to factorize.

So I sympathize with the bad feeling of repetition, but I’m not quite
sure how this can be avoided in this case.

WDYT?

Thanks,
Ludo’.

[bug#38460] [PATCH 1/1] lint: Add '--load-path' option.

[zimoun]

Hi Ludo,

On Sat, 7 Dec 2019 at 23:51, Ludovic Courtès <ludo@gnu.org> wrote:

> Well, technically speaking it’s not copy/pasted since the code you sent
> uses ‘find’ to grab the option; plus, we’re talking about very few
> lines, which is typically hard to factorize.

I agree. Even if I do not have a better solution to propose, I just
feel the current one is not optimal.

Does the same gettext entries G_ are duplicated? Do translators
translate 2 times (or more) the same string?


> So I sympathize with the bad feeling of repetition, but I’m not quite
> sure how this can be avoided in this case.
>
> WDYT?

Thank you for the explanations. I am not sure neither and need to fail
by myself to be convinced. ;-)
And currently, any hypothetical break should be reported by the test
suite. Hope so.


All the best,
simon

bug#38460: [PATCH 1/1] lint: Add '--load-path' option.

[Ludovic Courtès]

Hi,

zimoun <zimon.toutoune@gmail.com> skribis:

> On Sat, 7 Dec 2019 at 23:51, Ludovic Courtès <ludo@gnu.org> wrote:
>
>> Well, technically speaking it’s not copy/pasted since the code you sent
>> uses ‘find’ to grab the option; plus, we’re talking about very few
>> lines, which is typically hard to factorize.
>
> I agree. Even if I do not have a better solution to propose, I just
> feel the current one is not optimal.
>
> Does the same gettext entries G_ are duplicated? Do translators
> translate 2 times (or more) the same string?

No, they’re deduplicated.

>> So I sympathize with the bad feeling of repetition, but I’m not quite
>> sure how this can be avoided in this case.
>>
>> WDYT?
>
> Thank you for the explanations. I am not sure neither and need to fail
> by myself to be convinced. ;-)
> And currently, any hypothetical break should be reported by the test
> suite. Hope so.

I think so.

I’ve applied it now, thank you!

Ludo’.