[PATCH 01/10] guix: import: Print the number of packages at the end.

---

this will be a series of patches that were needed to be able to

(mostly) successfully run these two imports (go-ethereum and

ethersphere/bee):

RUN=12

clear && ./pre-inst-env guix import go -r --pin-versions github.com/ethersphere/bee@v1.5.1 > >(tee -a ~/workspace/guix/importing/bee-run-${RUN}.scm) 2> >(tee -a ~/workspace/guix/importing/bee-run-${RUN}.log >&2)

RUN=36

clear && ./pre-inst-env guix import go -r --pin-versions github.com/ethereum/go-ethereum@v1.10.17 > >(tee -a ~/workspace/guix/importing/go-ethereum-run-${RUN}.scm) 2> >(tee -a ~/workspace/guix/importing/go-ethereum-run-${RUN}.log >&2)

note that i only have a mediocre knowledge of the golang

infrastructure, so this should be reviewed by someone who

more deeply understands the golang build process, and its

implementation within guix.

i think most of it is not very controversial, maybe except

the last commit.

i'm willing to reshape this under the guidance of someone who

has a better bird's eye view perspective on this all.

guix/scripts/import.scm | 12 ++++++++----

1 file changed, 8 insertions(+), 4 deletions(-)

To properly mimic the name of the website. Also employ it at one more place.

---

guix/import/go.scm | 14 ++++++++------

1 file changed, 8 insertions(+), 6 deletions(-)

Introduce a (local) mockup logger, so that we don't need to keep adding and

deleting format's when debugging the codebase.

---

guix/import/go.scm | 11 +++++++++++

1 file changed, 11 insertions(+)

of URLs if they are plain strings.

---

guix/import/go.scm | 27 ++++++++++++++++++++-------

1 file changed, 20 insertions(+), 7 deletions(-)

This is needed when dealing with golang packages, as per:

A page may return 404, but at the same time also contain the sought after

`go-import` meta tag. An example for such a project/page is:

It's not enough to just handle the thrown exception, because we need to be

able to get hold of the fetched content, too.

Discussion why it's duplicated here: https://issues.guix.gnu.org/54836

accept-all-response-codes? param.

---

guix/import/go.scm | 107 ++++++++++++++++++++++++++++++++++++++++++++-

1 file changed, 105 insertions(+), 2 deletions(-)

When a version is specified in a replace directive, then we must also match

the version against any potential replacements.

---

guix/import/go.scm | 53 +++++++++++++++++++++++++++++++---------------

1 file changed, 36 insertions(+), 17 deletions(-)

This also adds logging statements to various parts of the go importer.

case of network errors. Add ability to enable printing a full backtrace when

debugging.

(go-module->guix-package): Tolerate the inability to fetch the synopsis and

description.

---

guix/import/go.scm | 104 +++++++++++++++++++++++++++++++++++++--------

1 file changed, 86 insertions(+), 18 deletions(-)

Handle golang projects that are located in a subdir relative to the repo's

root directory, and whose tags are also prefixed accordingly with the relative

path.

An example: guix import go github.com/aws/aws-sdk-go-v2/service/route53@v1.1.1

Also be more resilient while dealing with licences.

separated lists of licences, and tolerate the inability to fetch any license

information. Handle go modules that are a subdirectory of a vcs repo,

including the special naming of their tags (it includes the subdir as a prefix).

(list->licenses): Warn when the conversion fails.

(+known-vcs+): Renamed from known-vcs to use naming convention for constants.

---

guix/import/go.scm | 130 +++++++++++++++++++++++++++++++++++----------

1 file changed, 102 insertions(+), 28 deletions(-)

---

guix/import/go.scm | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

(*module-path->import-path*): New mapping, initialized with a small db of

projects.

(go-module->guix-package): Return packages with #:unpack-path when needed.

---

guix/import/go.scm | 93 +++++++++++++++++++++++++++-------------------

1 file changed, 54 insertions(+), 39 deletions(-)

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Why are unknown errors being catched here?

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

What's the '-continuable' for here? Is it to avoid extra 'raise-

exception' entries in the backtrace, or does this actually make use of

Scheme's continuable exceptions?

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:16 [+0200]:

What's the use case for counting the number of packages?

Why only for the Go importer, and why a warning? 

The 'info' macro from (guix diagnostics) seems to fit here (it also

uses 'current-warning-port', but only internally).

The plural form of 'packages' is incorrect when count=1.

Greetings,

Maxime

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Standard capitalisation is "GitHub" here, not that it really matters

much.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

We already have a logging functionality: 'info' from

guix/diagnostics.scm. No 'debug' yet though ...

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

You can use 'pk' for this:

#;(pk "inside replace, directive" directive "requirements" requirements)

(delete 'pk' when debugging).

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

There's some extra complexity here, so I recommend some corresponding

extra tests, to avoid it breaking in the future.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

How can a version be a list?

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Nevermind, there's some ...

Still, why choose the first old-version instead of the latest old-

version? How can we know which one is correct?.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Do we know for sure here that 'text' is actually a string? What if the

input was (strong (em (a (href "http://super") "Super") emphasis")?

I'm not familiar enough with sxml to be sure, but maybe the , can be

removed here.

Anyway, more cases are nice, but I recommend tests.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Ok, but couldn't this be done more generally, for all importers using

HTTP? E.g., could we have a variant 'http-fetch/retrying' of 'http-

fetch' that automatically retries a number of times? Then the retrying

functionality could almost trivially be used in other HTTP-using

importers as well.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Why a warning instead of a 'report-error'?

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Why only for the Go importer and not other importers?

Maybe the behaviour of other importers in presence of unidentifiable

licenses could be unified, with some common support procedure or

something. Then only a single location needs to be modified to change

things, and all importers would benefit.

I don't think this is correct, in the past I've put '(foobar) and

#false in 'license' fields of packages and those packages could be

compiled, without any compilation issues. I assume the same holds for

strings as well.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

The naming convention in (Guile) Scheme is '%known-vcs' or 'known-vcs'.

Also, 'define-constant' doesn't exist in Guile, and the Lisp macro

'defconstant' and 'defmacro' appears to have some complications w.r.t.

phasing. What would be the benefit of 'define-constant' here?

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Why a newline at the beginning instead of the end? Also, why the

logging? If I do "guix import go foobar", wouldn't I know in advance

it will process the foobar module? And in case of --recursive,

wouldn't I know after the fact which modules were processed?

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

You can use (and COND (substring ...)) here.

Also, the usual extra complexity --> extra tests.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Why a $ at the end but no "^" at the beginning? What's the point of

the leading ".*"? Could the regexp be constructed in advance, or only

once (maybe 'delay/force' and a global variable) -- turning a regexp

into a NFA into a DFA can be expensive.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Are you sure you want to ignore _all_ exceptions? Including stack

overflows, out-of-memories, unbound variable errors, type errors ...?

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

In some other location, the symbol 'unknonw-license!' is used. Here, a

string is used. Why the inconsistency? Also, given the "unknown-

license!", the warning seems mostly redundant to me.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

When can it be null? Also, tests.

Is there a need for speed here? If not, I'd keep things simple by

using the non-mutating version (I expect the speed gain to be neglible

here). If there is, I would also use the mutating version of map --

'map!' from srfi-1.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

Then they need to be recorded, and the reviewer will ask why tests are

being skipped here.

Greetings,

Maxime.

*all* errors are caught, displayed, and swallowed, so that the importing can proceed to the rest of the packages.

a possible scenario: a large import can take several minutes. if there's a transient network error, then this way it may finish with 99% of the packages, and the rest can be restarted by hand after inspecting the log output.

another scenario is that the importer is simply buggy, and the user gets 99% of the packages imported, and has to do only one package by hand.

honestly, i can't remember. which translates to: i should have

commented on that!

*shakes head and makes a mental note*

from guile's manual:

"If continuable? is true, the handler is invoked in tail position

relative to the raise-exception call. Otherwise if the handler

returns, a non-continuable exception of type &non-continuable is

raised in the same dynamic environment as the handler."

i.e. it should rather be called raise-continuably not

raise-continuable.

and i think the reason is that the stack is not unwound that way, so

that the handlers in the guard can print a backtrace showing the

entire stack.

does this make sense? i'm still learning scheme's stack/exception

handling...

or maybe what i wanted is that the handlers in the guard can return

with (values #f '())? but i think that should work with vanilla RAISE,

too.

because i want the importer to be able to deal with a transitive

closures of dependencies with > 400 entries. if it fails at the first

error, then a `guix import go -r` becomes a hopeless endeavor for

larger go modules.

thanks for the review Maxime! i'll address the rest, too.

--

• attila lendvai

• PGP: 963F 5D5F 45C7 DFCD 0A39

--

“'Emergencies' have always been the pretext on which the safeguards of individual liberty have been eroded.”

— F. A. Hayek (1899–1992)

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:

This seems implied by typing "guix import go --recursive foo@bar"?

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:16 [+0200]:

FWIW (given that I don't know the Go packaging system well), the last

patch (10/10) seems the least controversial to me. I'm not seeing why

it would be controversial.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 17:00 [+0000]:

Catching (presumably) transient network errors and reporting them with

'report-network-error' and the like: no problem.

... but if there's some kind of bug, why not just report and fix it?

If the bug is fixed, then no packages need to be done by hand and other

people will benefit as well. Whereas if it's only for 1% and the

backtrace only happens for recursive import and not for non-recursive

or manual import, then I'd think that the chance that the user actually

reports or fixes the bug decreases a lot.

Greetings,

Maxime.

realistically? because there are years old issues, and long bitrotten

patches in every project's issue tracker, Guix included.

i think an importer, that does proper logging, and finishes with a 99%

correct result, is worth much more than one that barks at the first

error, and then refuses to continue processing the 100+ remaining

entries.

this was true even in my own case, when i also had the maintainer hat

on, i.e. i was already neck deep into fixing the importer.

the tradeoff is this: what leads to a lower level of overall annoyance of all the parties involved:

1) have a tool that is less useful, and through that it annoys the

users to report issues, which then (hopefully) thrusts some people

towards improving the tool... or

2) have a tool that does its best to produce the most useful output.

short of hard data, this can be argued forever. my intuition is clearly pointing towards 2), because i doubt that the choking point of improving the importer is due to not having enough examples on which it breaks.

does it make enough sense to justify my proposed behavior?

--

• attila lendvai

• PGP: 963F 5D5F 45C7 DFCD 0A39

--

“Communism doesn’t work because people like to own stuff.”

— Frank Zappa (1940–1993)

because it introduces an in-source database of golang projects that

require special-casing (*module-path->import-path*).

i suspect that whatever info i'm storing in that db of exceptions is

probably available somewhere, somehow.

--

• attila lendvai

• PGP: 963F 5D5F 45C7 DFCD 0A39

--

“Nobody in the world, nobody in history, has ever gotten their freedom by appealing to the moral sense of the people who were oppressing them.”

— Assata Shakur (1947–)

it makes the log more readable.

here's how i was working on this codebase:

RUN=12

clear && ./pre-inst-env guix import go -r --pin-versions github.com/ethersphere/bee@v1.5.1 > >(tee -a ~/workspace/guix/importing/bee-run-${RUN}.scm) 2> >(tee -a ~/workspace/guix/importing/bee-run-${RUN}.log >&2) && beep

RUN=36

clear && ./pre-inst-env guix import go -r --pin-versions github.com/ethereum/go-ethereum@v1.10.17 > >(tee -a ~/workspace/guix/importing/go-ethereum-run-${RUN}.scm) 2> >(tee -a ~/workspace/guix/importing/go-ethereum-run-${RUN}.log >&2) && beep

IOW, i was repeatedly firing long-running imports whose output i stored in versioned files. when a run was finished, i inspected the log and the output file (containing hundreds of packages).

whenever i had to understand what went wrong -- which often manifested in successfull runs producing broken output (i.e. no errors/backtraces) --, i added log statements to see what's happening. i could have deleted the printfs, and then re-add them again a gazillion times, or just leave them there for the upcoming programmers (which includes me), conditionally compiled in with the help of a macro.

background: in my lisp codebase emacs colors the log statement gray, and the logging lib i'm using in CL has a separate compile-time and a runtime log level. IOW, i can eliminate the entire runtime logging overhead, and most of the code readability overhead.

--

• attila lendvai

• PGP: 963F 5D5F 45C7 DFCD 0A39

--

“Associate yourself with people of good quality, for it is better to be alone than in bad company.”

— Booker T. Washington (1856–1915)

Attila Lendvai schreef op ma 09-05-2022 om 12:34 [+0000]:

(For a positive note, skip to the last paragraph!)

If we do something like this (*) (this = catching errors for

dependencies and skipping those packages), I don't want this to be

something Go-specific. Could this be done in 'recursive-import' or

something like that instead, such that all importers benefit and Go

isn't some weird special case?

Also, given that we appear to have a number of people effectively

maintaining the Go importer ("git log guix/import.go.scm" mentions a

few ‘Maxim Cournoyer’, ‘Ludovic Courtès’ ‘Xinglu Chen’ and ‘Sarah

Morgensen’, and more recently maybe you?) and I don't see any open old

Go importer bugs/patches on issues.guix.gnu.org, I don't think we have

to worry about

in this case.

If you know of some examples on which the Go importer broke, could you

add them to tests/go, to make sure it won't break again later

(regression), in case of future changes?

Anyway, let's take a look at for which kind of things I think of in

case of ‘buggy importer’:

1. (Maybe [PATCH 04/10]?) The parser for a file format cannot parse 

<something>, so it throws an exception.

(I consider this more a ‘limitation’ than a ‘bug’.)

For this kind of thing, maybe we can just throw a ‘oops I don't

understand the file format’ exception, report an appropriate error

message (package name + maybe line number, but no need for a

backtrace), skip that package and continue. That way, the Go

importer will be to some degree robust to future changes.

That's already done in this patch series, except for the ‘no need

for a backtrace’ part and this patch series catching all exceptions

instead of just networking errors, ‘I don't understand the file

format’ errors, ...

2. The Go importer misinterprets some kind of field or such.

These kind of things don't (typically?) cause errors, so skipping.

(Still a bug though!)

3. The Go importer cannot produce a package definition because it

doesn't support <some Go packaging method>.

Similar (1).

4. The Go importer has some kind of type error or such.

This case looks like an actual bug to me, where I think that

catching the resulting exceptions is just sweeping things under

the rug, implicitly teaching other people that it's ok to just

ignore problems without making any attempt of fixing them or

reporting them. Vaguely similar to ‘broken windows’

(https://en.wikipedia.org/wiki/Broken_windows_theory).

Sure, for a single particular user and short term this makes the

importer a bit less useful, but knowing bugs is a prerequisite for

fixing them, and long term, by actually knowing about the bugs and

fixing them, more users will benefit.

That said, like "guix build" has a "--keep-going" option to temporarily

ignore to some degree packages that fail to build, maybe "guix import"

(when using --recursive) can have a "--keep-going" option.

Greetings,

Maxime.

note that the list of people *using* the go importer is probably not all that much larger.

the fact that the go importer bugs were not even reported into the issue tracker doesn't change much about the fact that i have experienced several problems when i first tried to recursively import a non-trivial go project (which even included issues with http-fetch). it means that all those bug were there, not hidden by exception handling, and yet they have *not* been reported into the issue tracker.

or to put it another way, i think there are just too many unverified assumptions in this discussion about actual user behavior. my strategy in those situations is to expose as much info as feasible, and let the users respond intelligently to the situation.

note that this patch does not just catch errors, but handles them by emitting useful information about them, and continues only after that.

i consider silently swallowing errors a major sin (read: code that will certainly eat up some portion of someone's life in the future).

the only place where this patch silently swallows errors is in the extraction of descriprions and such, i.e. data that the user must review anyway.

this patch does not hide any bugs, only enables the code to continue by skipping a package.

and as a bonus: i wouldn't be surprised if it emitted *more* useful information than when allowing the exception to pass through to the caller, because error handling in the guix codebase could use a bit more love.

ok, i'll look into putting this feature behind a CLI argument, and generalize it to all importers. as time allows i'll get back to the rest of the questions/suggestions, too.

thanks for the feedback Maxime!

--

• attila lendvai

• PGP: 963F 5D5F 45C7 DFCD 0A39

--

“A great civilization is not conquered from without until it destroys itself from within.”

— Will Durant

Attila Lendvai schreef op ma 09-05-2022 om 20:02 [+0000]:

I think such errors are easily overlooked, but yes, and I don't have

any actual data on that.

I think I've overstated the ‘swallows errors etc.’ a bit, given that

if "guix import go --recursive <foo>" skips a package <bar>, then the

user will have to package its dependency <bar> anyway, and for that

the user could first try "guix import go <bar>" (reporting errors on

bug-guix@ or #guix) before fixing/extending the importer or doing

things manually.

Greetings,

Maxime.

Attila Lendvai schreef op di 03-05-2022 om 13:43 [+0200]:

In (Guile) Scheme (at least as used in the Scheme code I've seen),

*earmuffs* are typically used for mutable variables, not for global

constants. Though guix/self.scm uses *system-modules* for not-really-

constant-but-not-mutated lexical variables so maybe ...

FWIW, I would write (define %module-path->import-path ...) here, not

that it really matters.

About the ‘is this controversional’ bit: these kind of mappings and

sets have precedent, see e.g. 'known-vcs' in (guix import cran),

%builtin-mods in (guix import minetest), default-r-packages in (guix

import cran).

Greetings,

Maxime.

Hi,

I forgot that "guix build" already does some debug logging when passed

"--verbose" or "--debug". Maybe "guix import ..." can gain a similar 

system. I don't know if the internals are reusable though ...

(I'm not convinced that we need logging for "guix import" but I'm not

convinced of the contrary either, and there is already some kind of

debug logging system elsewhere.)

FWIW, for these kind of things I do

while ./pre-inst-env guix some-command; do echo 'Next'; done

and for your case (broken output without errors), maybe

while true; do echo 'Next run' >> the-log-file && ./pre-inst-env guix some-command >> the-log-file; done

Greetings,

Maxime.

I'm not sure if I mentioned it previously, but it looks like some

patches are uncontroversial and usable mostly as-is (e.g. 04/10 without

the logging, 02/10, 05/10 with verifying types, 06/10 without the

overly-general false-if-exception and logging, 9/10, 10/10, 04/10). So

you could try the non-controversial bits first and then see if some

kind of consensus or such could be found for the remainder.

Greetings,

Maxime.

Hello Attila,

Attila Lendvai <attila@lendvai.name> writes:

We alreading have 'warning' and 'info' in (guix diagnostics); perhaps we

could also have 'debug', enabled when GUIX_DEBUG is set or something.

Alternatively, there's a full blown logging library available in the

Guile-Lib project, as (logging logger) [0].

[0] https://www.nongnu.org/guile-lib/doc/ref/logging.logger/

Maxim