| Message ID | da08c31cbea9bc194bf1eea68c3c532e28628310.1646227054.git.seerlite@nixnet.email |
|---|---|
| State | New |
| Headers | show |
| Series | vim: Detect plugins via search paths. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
SeerLite via Guix-patches via schreef op wo 02-03-2022 om 10:17 [- 0300]: > + (add-before 'check 'set-environment-variables > + (lambda* (#:key inputs #:allow-other-keys) > + ;; One of the tests tests timezone-dependent functions. > + (setenv "TZDIR" > + (search-input-directory inputs "share/zoneinfo")) > > - ;; Make sure the TERM environment variable is set for the tests > - (setenv "TERM" "xterm"))) The tzdata in in 'native-inputs', not 'inputs', so this code needs to look in '(or native-inputs inputs)', not 'inputs', otherwise this code will fail with a '&search-path' exception from 'search-input-directory' when cross-compiling (untested). Greetings, Maxime.
SeerLite via Guix-patches via schreef op wo 02-03-2022 om 10:17 [- 0300]: > + (lambda* (#:key inputs #:allow-other-keys) > + [this [bracketed line] seems irrelevant for my comment] > + (substitute* '("src/testdir/Makefile" > + "src/testdir/test_normal.vim" > + "src/testdir/test_popupwin.vim" > + "src/testdir/test_shell.vim" > + "src/testdir/test_system.vim" > + "src/testdir/test_terminal.vim" > + "src/testdir/test_terminal2.vim") > + (("/bin/sh") (search-input-file inputs "bin/sh"))) > + (substitute* "src/testdir/test_autocmd.vim" > + (("/bin/kill") (search-input-file inputs "bin/kill"))))) This is test stuff, and these binaries do not seem to be present in 'inputs', they would be in the implicit 'native-inputs', so these would need to search in '(or native-inputs inputs)' instead of 'inputs' to avoid &search-path exceptions when cross-compiling: (substitute* '("src/testdir/...") (("/bin/sh") (search-input-file (or native-inputs inputs) "bin/sh"))) Or simpler, there's a procedure for looking for 'bin/TOOL' in native- inputs: 'which'! ;; the original code! (substitute* '("src/testdir/...") (("/bin/sh") (which "sh"))) Why the change from 'which' to 'search-input-file'? Greetings, Maxime.
SeerLite via Guix-patches via schreef op wo 02-03-2022 om 10:17 [- 0300]: > + (add-before 'install 'fix-installman.sh > + (lambda* (#:key inputs #:allow-other-keys) > + (substitute* "src/installman.sh" > + (("/bin/sh") > + (search-input-file inputs "bin/sh"))))) Is "installman.sh" used during the build, or when vim is run? In the former case, this should look into (or native-inputs inputs) instead for cross-compilation reasons, and could be simplified to: (("/bin/sh") (which "sh")) Greetings, Maxime.
Hi, thanks for the review! On 3/2/22 14:29, Maxime Devos wrote: > This is test stuff, and these binaries do not seem to be present in > 'inputs', they would be in the implicit 'native-inputs', so these would > need to search in '(or native-inputs inputs)' instead of 'inputs' to > avoid &search-path exceptions when cross-compiling: > > (substitute* '("src/testdir/...") > (("/bin/sh") (search-input-file (or native-inputs inputs) "bin/sh"))) > > Or simpler, there's a procedure for looking for 'bin/TOOL' in native- > inputs: 'which'! > > ;; the original code! > (substitute* '("src/testdir/...") > (("/bin/sh") (which "sh"))) Whoops, I forgot I made this change. > Why the change from 'which' to 'search-input-file'? The blog post that introduces label-less inputs also introduces 'search-input-file', which made me think they were both part of the "package definition modernization process". I asked on IRC if that was the case, and although I didn't get a clear answer for that, someone told me they preferred using 'search-input-file' because it raises an exception when no file is found. What do you think about that? Should I stick with 'search-input-file' or is 'which' alright? It makes sense that I'd have to use native-inputs though. My bad!
Thank you again for the corrections. Should I send the entire patch series again or only the patches I want to correct? I'm new to the whole debbugs/email patch workflow.
SeerLite schreef op do 03-03-2022 om 13:49 [-0300]: > The blog post that introduces label-less inputs also introduces > 'search-input-file', which made me think they were both part of the > "package definition modernization process". The problem with 'which' was that it looks in 'native-inputs' (+ inputs when compiling natively), even when we need 'inputs'. To fix this, adding an 'inputs/native-inputs' argument was proposed (https://issues.guix.gnu.org/47869). That interface was considered a bit weird, so then (string-append (assoc-ref ...) "bin/...") was proposed. That's rather verbose though, so 'search-input-file' was introduced. In short, 'search-input-file' was introduced to help with locating binaries from 'inputs' (at least from my POV, shortly after ludo began using 'search-input-file' for delabelification). For looking in native-inputs (compilation tools etc.), I would prefer 'which' above the 'search-input-file' because it's a lot more concise, though I suppose there is an argument to be made for using 'search- input-file' for consistency. There's the downside of not raising an exception in case the file is not found though. However, there appears to be very few code that actually relies on the 'return #f' behaviour (search for '(or (which' and '(if (which'), so I think the behaviour can be changed without much trouble (on 'core-updates'). Greetings, Maxime.
Hi Seerlite, SeerLite <seerlite@nixnet.email> skribis: > Thank you again for the corrections. Should I send the entire patch > series again or only the patches I want to correct? I'm new to the > whole debbugs/email patch workflow. To avoid confusion, it’s more convenient if you resend the entire patch series; make sure to use ‘git format-patch --subject-prefix="PATCH v2"’. Thank you! Ludo’.
Hi! Sorry for the delay, I've been very busy. I sent the rebased patches but only the bits that added search paths. There were some updates to both Vim and Neovim since I sent the first patch and I didn't want to deal with fixing the conflicts and re-understanding the style changes I had even forgotten I made. So I'm sending just the functional parts of the patch and hopefully someone else can update the package styles if necessary :) Oh and I'd like to remind you of this patch: <https://issues.guix.gnu.org/48112> which along with my patch would make it so Neovim can finally use both Vim and Neovim-exclusive plugins. I'd appreciate if it got merged too. SeerLite
Rebased on top of 182b25fb (<https://issues.guix.gnu.org/55045>) and fixed the wrong lookup for tzdata files in inputs instead of native-inputs again, as Maxime had pointed out (something I thought was caused by my previous style/gexp patch when it was not). I wasn't able to figure out how to test it though. SeerLite
diff --git a/gnu/packages/vim.scm b/gnu/packages/vim.scm index 3997797201..d5d8b412f7 100644 --- a/gnu/packages/vim.scm +++ b/gnu/packages/vim.scm @@ -89,68 +89,69 @@ (define-public vim "1jppzgmngcdd7jfb5rnkkvf5d47svnjbn7qj4mvjacd9az3c7s9r")))) (build-system gnu-build-system) (arguments - `(#:test-target "test" - #:parallel-tests? #f - #:phases - (modify-phases %standard-phases - (add-after 'configure 'patch-absolute-paths - (lambda _ - (substitute* "runtime/tools/mve.awk" - (("/usr/bin/nawk") (which "gawk"))) - (substitute* '("src/testdir/Makefile" - "src/testdir/test_normal.vim" - "src/testdir/test_popupwin.vim" - "src/testdir/test_shell.vim" - "src/testdir/test_system.vim" - "src/testdir/test_terminal.vim" - "src/testdir/test_terminal2.vim") - (("/bin/sh") (which "sh"))) - (substitute* "src/testdir/test_autocmd.vim" - (("/bin/kill") (which "kill"))))) - (add-before 'check 'set-environment-variables - (lambda* (#:key inputs #:allow-other-keys) - ;; One of the tests tests timezone-dependent functions. - (setenv "TZDIR" - (search-input-directory inputs "share/zoneinfo")) + (list + #:test-target "test" + #:parallel-tests? #f + #:phases + #~(modify-phases %standard-phases + (add-after 'configure 'patch-absolute-paths + (lambda* (#:key inputs #:allow-other-keys) + (substitute* "runtime/tools/mve.awk" + (("/usr/bin/nawk") (search-input-file inputs "bin/gawk"))) + (substitute* '("src/testdir/Makefile" + "src/testdir/test_normal.vim" + "src/testdir/test_popupwin.vim" + "src/testdir/test_shell.vim" + "src/testdir/test_system.vim" + "src/testdir/test_terminal.vim" + "src/testdir/test_terminal2.vim") + (("/bin/sh") (search-input-file inputs "bin/sh"))) + (substitute* "src/testdir/test_autocmd.vim" + (("/bin/kill") (search-input-file inputs "bin/kill"))))) + (add-before 'check 'set-environment-variables + (lambda* (#:key inputs #:allow-other-keys) + ;; One of the tests tests timezone-dependent functions. + (setenv "TZDIR" + (search-input-directory inputs "share/zoneinfo")) - ;; Make sure the TERM environment variable is set for the tests - (setenv "TERM" "xterm"))) - (add-before 'check 'skip-or-fix-failing-tests - (lambda _ - ;; This test assumes that PID 1 is run as root and that the user - ;; running the test suite does not have permission to kill(1, 0) - ;; it. This is not true in the build container, where both PID 1 - ;; and the test suite are run as the same user. Skip the test. - ;; An alternative fix would be to patch the PID used to a random - ;; 32-bit value and hope it never shows up in the test environment. - (substitute* "src/testdir/test_swap.vim" - (("if !IsRoot\\(\\)") "if 0")) + ;; Make sure the TERM environment variable is set for the tests + (setenv "TERM" "xterm"))) + (add-before 'check 'skip-or-fix-failing-tests + (lambda _ + ;; This test assumes that PID 1 is run as root and that the user + ;; running the test suite does not have permission to kill(1, 0) + ;; it. This is not true in the build container, where both PID 1 + ;; and the test suite are run as the same user. Skip the test. + ;; An alternative fix would be to patch the PID used to a random + ;; 32-bit value and hope it never shows up in the test environment. + (substitute* "src/testdir/test_swap.vim" + (("if !IsRoot\\(\\)") "if 0")) - ;; These tests check how the terminal looks after executing some - ;; actions. The path of the bash binary is shown, which results in - ;; a difference being detected. Patching the expected result is - ;; non-trivial due to the special format used, so skip the test. - (substitute* "src/testdir/test_terminal.vim" - ((".*Test_open_term_from_cmd.*" line) - (string-append line "return\n")) - ((".*Test_terminal_postponed_scrollback.*" line) - (string-append line "return\n")) - ((".*Test_combining_double_width.*" line) - (string-append line "return\n"))) - (substitute* "src/testdir/test_popupwin.vim" - ((".*Test_popup_drag_termwin.*" line) - (string-append line "return\n"))))) - (add-before 'install 'fix-installman.sh - (lambda _ - (substitute* "src/installman.sh" - (("/bin/sh") - (which "sh"))))) - (add-after 'install 'install-guix.vim - (lambda* (#:key inputs outputs #:allow-other-keys) - (let ((vimdir (string-append (assoc-ref outputs "out") "/share/vim"))) - (mkdir-p vimdir) - (copy-file (assoc-ref inputs "guix.vim") - (string-append vimdir "/vimrc")))))))) + ;; These tests check how the terminal looks after executing some + ;; actions. The path of the bash binary is shown, which results in + ;; a difference being detected. Patching the expected result is + ;; non-trivial due to the special format used, so skip the test. + (substitute* "src/testdir/test_terminal.vim" + ((".*Test_open_term_from_cmd.*" line) + (string-append line "return\n")) + ((".*Test_terminal_postponed_scrollback.*" line) + (string-append line "return\n")) + ((".*Test_combining_double_width.*" line) + (string-append line "return\n"))) + (substitute* "src/testdir/test_popupwin.vim" + ((".*Test_popup_drag_termwin.*" line) + (string-append line "return\n"))))) + (add-before 'install 'fix-installman.sh + (lambda* (#:key inputs #:allow-other-keys) + (substitute* "src/installman.sh" + (("/bin/sh") + (search-input-file inputs "bin/sh"))))) + (add-after 'install 'install-guix.vim + (lambda _ + (let ((vimdir (string-append #$output "/share/vim")) + (vimrc #$(local-file (search-auxiliary-file "guix.vim")))) + (mkdir-p vimdir) + (copy-file vimrc (string-append vimdir "/vimrc")))))))) (native-search-paths (list (search-path-specification (variable "GUIX_VIMRUNTIME") @@ -159,11 +160,7 @@ (define-public vim (inputs (list gawk ncurses perl tcsh)) ; For runtime/tools/vim32 (native-inputs - `(("libtool" ,libtool) - ("guix.vim" ,(search-auxiliary-file "guix.vim")) - - ;; For tests. - ("tzdata" ,tzdata-for-tests))) + (list libtool tzdata-for-tests)) (home-page "https://www.vim.org/") (synopsis "Text editor based on vi") ;; The description shares language with the vim-full package. When making