| Message ID | 20210119154525.11230-1-zimon.toutoune@gmail.com |
|---|---|
| Headers | show |
Hi! zimoun <zimon.toutoune@gmail.com> skribis: > This corner case #1 happens when the package does not exist; then the function > 'lookup-node' is not able to "unpack" the 'values' and throw and ugly > backtrace, as exposed in bug#44114 <http://issues.guix.gnu.org/44115#3>. > > With these trivial patches, it is fixed for all the importers except 'opam' > (because of 'and-let' which needs some care). Neat! > Now, instead of throwing an ugly backtrace, it simply say almost nothing: > > $ ./pre-inst-env guix import cran do-not-exist -r > error: failed to retrieve package information from "https://cran.r-project.org/web/packages/do-not-exist/DESCRIPTION": 404 ("Not Found") > > $ ./pre-inst-env guix import pypi do-not-exist -r > following redirection to `https://pypi.org/pypi/do-not-exist/json/'... > #f > > > This non-existent message is because the error is poorly handled. With the 4 > patches, the situation is the same as "guix import gem" for all the importers > with the recursive option. One way for a better error handling is done in the > last commit for 'guix import gem' only; the same trick can be done for all. > > $ guix import gem do-not-exist -r > #f > > $ ./pre-inst-env guix import gem do-not-exist -r > guix import: error: failed to download meta-data for package 'do-not-exist' I think we do want this error message. Why should we ignore non-existent packages when doing ‘-r’? It would think they’re still a problem, no? > In my opinions, UI messages should not appear in guix/import/*.scm but only in > guix/scripts/*.scm. I agree with the general idea, though sometimes taking this shortcut is beneficial (maybe not in this case?). > If I understand correctly, then the way the errors are reported could be > uniformized between all the importers, and maybe the snippet in the subcommands > "guix import <from>" could be refactorized a bit. Probably. ‘-r’ started as an option specific to one importer, but now that most of them (?) support it, it’d make sense to rethink the interfaces. Thanks for looking into it! Ludo’.
Hi Ludo, Thanks for the review. On Tue, 26 Jan 2021 at 23:24, Ludovic Courtès <ludo@gnu.org> wrote: >> $ guix import gem do-not-exist -r >> #f >> >> $ ./pre-inst-env guix import gem do-not-exist -r >> guix import: error: failed to download meta-data for package 'do-not-exist' > > I think we do want this error message. Why should we ignore > non-existent packages when doing ‘-r’? It would think they’re still a > problem, no? Do you mean instead of displaying an error about the query (what the patch does), displaying which dependent package has failed? Something along these lines: $ ./pre-inst-env guix import gem foo -r guix import: error: failed to download meta-data for package 'bar' If it is what you have in mind, it needs to really re-think how ’recursive-import’ works. Not only fixing the corner case. :-) >> If I understand correctly, then the way the errors are reported could be >> uniformized between all the importers, and maybe the snippet in the subcommands >> "guix import <from>" could be refactorized a bit. > > Probably. ‘-r’ started as an option specific to one importer, but now > that most of them (?) support it, it’d make sense to rethink the > interfaces. If we agree on the first part (the function argument “#:key repo->guix-package” provided to ’recursive-import’ should always return ’values’), then let rethink the interface and how the errors are handled. Cheers, simon
Hi, zimoun <zimon.toutoune@gmail.com> skribis: > zimoun (5): > import: pypi: Return 'values'. > import: hackage: Return 'values'. > import: elpa: Return 'values'. > import: cran: Return 'values'. > scripts: import: gem: Fix recursive error handling. Finally pushed, a year later: 5278cab3dc scripts: import: gem: Fix recursive error handling. 7229b0e858 import: cran: Return multiple values for unknown packages. 1fe81b349c import: elpa: Return multiple values for unknown packages. 6bb92098b4 import: hackage: Return multiple values for unknown packages. 434925379d import: pypi: Return multiple values for unknown packages. ebb03447f8 import: pypi: Gracefully handle missing project home page. My questions back then were probably unnecessary; we should have applied these patches right away, my apologies. Thanks, Ludo’.
Hi Ludo, Thanks for the review and the push. > > All in all, it is worth to rethink all that. Maybe let drop this patch > > set and I could come back with a clean fix. If no one beats me. :-) [...] > My questions back then were probably unnecessary; we should have applied > these patches right away, my apologies. Well, a complete revamp for a better error handling is still worth. Sorry, I have been lazy to finish to unknot all that. The situation is better and better with the importer though. :-) Cheers, simon
Dear, Currently, there is 2 issues: 1. <from>->guix-package returning #f instead of (values #f '()) 2. error incorrectly handled by guix/scripts/import/<from> This corner case #1 happens when the package does not exist; then the function 'lookup-node' is not able to "unpack" the 'values' and throw and ugly backtrace, as exposed in bug#44114 <http://issues.guix.gnu.org/44115#3>. With these trivial patches, it is fixed for all the importers except 'opam' (because of 'and-let' which needs some care). Now, instead of throwing an ugly backtrace, it simply say almost nothing: --8<---------------cut here---------------start------------->8--- $ ./pre-inst-env guix import cran do-not-exist -r error: failed to retrieve package information from "https://cran.r-project.org/web/packages/do-not-exist/DESCRIPTION": 404 ("Not Found") $ ./pre-inst-env guix import pypi do-not-exist -r following redirection to `https://pypi.org/pypi/do-not-exist/json/'... #f --8<---------------cut here---------------end--------------->8--- This non-existent message is because the error is poorly handled. With the 4 patches, the situation is the same as "guix import gem" for all the importers with the recursive option. One way for a better error handling is done in the last commit for 'guix import gem' only; the same trick can be done for all. --8<---------------cut here---------------start------------->8--- $ guix import gem do-not-exist -r #f $ ./pre-inst-env guix import gem do-not-exist -r guix import: error: failed to download meta-data for package 'do-not-exist' --8<---------------cut here---------------end--------------->8--- In my opinions, UI messages should not appear in guix/import/*.scm but only in guix/scripts/*.scm. If I understand correctly, then the way the errors are reported could be uniformized between all the importers, and maybe the snippet in the subcommands "guix import <from>" could be refactorized a bit. WDYT? All the best, simon zimoun (5): import: pypi: Return 'values'. import: hackage: Return 'values'. import: elpa: Return 'values'. import: cran: Return 'values'. scripts: import: gem: Fix recursive error handling. guix/import/cran.scm | 5 ++--- guix/import/elpa.scm | 7 ++----- guix/import/hackage.scm | 16 ++++++++++------ guix/import/pypi.scm | 10 +++++----- guix/scripts/import/gem.scm | 27 +++++++++++++++------------ 5 files changed, 34 insertions(+), 31 deletions(-) base-commit: 2d9c6542c804eb2ef3d8934e1e3ab8b24e9bbafb prerequisite-patch-id: 6ce76af47a26307f4b99162b5ae2b47f5e5f220f prerequisite-patch-id: 32b0ac51a8fbe72cc9204c5a6d0e2b987af7b7f6 prerequisite-patch-id: 3fa663069818b59ab4d324cc69fabcd62c5a9b50