Fix quickloading :defsystem-depends-on dependencies.

[attila-lendvai]

Requires at least ASDF 3.1.7, or maybe newer, depending
on when the ASDF fix will be released.

the relevant ASDF changes: https://gitlab.common-lisp.net/asdf/asdf/merge_requests/1

note: the diff is mostly whitespace, it just moves part of the computation inside the handler-case.

[quicklisp]

Is there a way to make this work with older ASDFs than 3.1.7?

[attila-lendvai]

hrm, good question. without that change ASDF signals a rather general error, so i'm afraid not, but @fare may have an idea.

[quicklisp]

I think the specific error is embedded in the general error, maybe that could be dug out?

[attila-lendvai]

oh, that's a good catch! i'm trying to do that now.

[attila-lendvai]

@xach i can get hold of the nested condition, but it contains a missing-dependency with:

parent: NIL, missing: :HU.DWIM.ASDF

any ideas how to be smart about it? the current ql code skips trying if parent is nil.

i assume it's because missing-dependency may be signalled for non-sytems, i.e. for a file that is part of an asdf system?

[quicklisp]

A missing file also signals a missing dependency, yes. Don't know how the two situations might easily be distinguished, except by the context in which they are signaled, maybe.

[attila-lendvai]

i don't have any sane ideas... :/

but FTR, i was sloppy with my wording: this patch doesn't require a recent ASDF per se. it will "merely" remain broken wrt :defsystem-depends-on with an ASDF that doesn't include a fix for the above mentioned ASDF bug.

[quicklisp]

Any insane ideas? I don't mind workarounds.

[attila-lendvai]

i was thinking of looking at the backtrace, but it's not that urgent... :)

then i checked asdf-cache, but the visible binding only has this entry: (ASDF/CACHE:GET-FILE-STAMP "/home/alendvai/common-lisp/projectured/projectured.sdl.test.asd").

with non-trivial efforts we could parse the defsystem and deal with the :defsystem-depends-on entry proactively, before asking ASDF to load the system. but this again smells like "this is not that urgent", and/or "it may be easier to patch, release and update ASDF".

[quicklisp]

Quicklisp dist creation already proactively indexes :defsystem-depends-on and preloads its contents, but it's imperfect and doesn't work on systems that aren't part of quicklisp, and doesn't even work that well on systems that are part of quicklisp. So I think it has to be caught and handled, somehow.

[attila-lendvai]

@xach i was wondering, is there a reason why ql tries to catch missing-dependency instead of hooking into find-system? AFAIU, that hook/machinery in ASDF was added exactly for such a usecase.

[attila-lendvai]

this patch is less tested than i would like it to be, but i publish it here for feedback.

[fare]

Attila, it would be much more robust if the very same sysdef-*-search function that handled already-downloaded QL systems would also do the downloading, etc. (possibly controlled by a special boolean variable). Indeed, only in THAT setup is it ensured that the priority of systems does not depend on the order in which they are downloaded or not. Otherwise, you may run into e.g. prebuilt systems that override the QL systems if not downloaded yet, but not if already downloaded.

[attila-lendvai]

updated the patch to have only one hook as @fare suggested.

in its current form it skips calling the entire compute-load-strategy machinery, and just prompts before each install (if prompting is requested). the reason is that c-l-s calls asdf:find-system again, and that triggers the dependency looping error.

but at least it works as expected, properly downloading :defsystem-depends-on, too.

[fare]

If you can come up with a simple test file that demonstrates what ASDF should be doing that it isn't doing (or shouldn't that it is), I can probably fix ASDF to behave properly.

Looking at a backtrace is insane, but ASDF could maintain in a special variable the information you need. What information do you need?

Right now, I'm not sure how I can help.

[fare]

It's OK for you to make up new API function in your test file, as long as you document what you think it should be doing.

[attila-lendvai]

concept failed with the find-system hook -- i think.

@fare ASDF calls SYSDEF-PRELOADED-SYSTEM-SEARCH (e.g. to find itself) after consulting the user registered hooks, so in an earlier hook I have no chance to know that the system will be found later on (and thus don't try to download anything).

I think the proper solution would be something like an asdf:*system-not-found-hooks*, or alternatively a signaled condition that can be properly identified by user code, and probably signaled from an ASDF state where calling back to ASDF may happen... i.e. we are back to square one, no?

unless maybe if ASDF could move its own search functions into the hooks and then i could register mine after them, which would be more or less equivalent to an asdf:*system-not-found-hooks*.

so, to sum it up: find-system hooks, in their current form, are not a reliable way to identify the situation when asdf:find-system gives up, which is exactly what ql needs to hook into.

[fare]

Proposed workaround:
1- ensure your hook is last in system-definition-search-functions
2- before you decide that asdf won't find the system, explicitly call sysdef-preloaded-system-search to see if ASDF is going to find it as preloaded.

[fare]

Regarding the failure at http://paste.lisp.org/display/159756
ASDF is specially re-loaded (a NOP if up to date) at the beginning of any ASDF operation. Looks like you'd trying to do use ASDF inside the call to find-system asdf, as part of your QL hook. If your move the QL hook to inside find-system, you may want to specially exclude asdf (and uiop?) from its magic processing.

[attila-lendvai]

ok, this is a non-intrusive fix for the original issue (ql not handling :defsystem-depends-on dependencies).

@xach i think this version is something that could land in the next version of ql.

@fare let us know if you think ql should hook into ASDF in a different way than this.

[fare]

Attila, I don't see anything wrong with this patch to QL as far as ASDF is concerned.

I still don't understand why it's not better and easier to "just" hook at the end of system-definition-search-functions with a special case to handle sysdef-preloaded-system-search.

[attila-lendvai]

dunno, i also don't see the reverse.

the reason i settled with this is that it's a less intrusive patch relative to what ql did previously. and i don't see how a hook would be any less coupled with ASDF internals, or in general a better solution. not saying i'm right, just that i don't see.

[fare]

Using the ASDF would make it "work by construction", and wouldn't require restarting things while potentially leaving incomplete side-effects in mid-air. Also, no special wrapper would be required, just a flag to enable or disable download.

[attila-lendvai]

ok, that sounds convincing.

so, here's the hook variant: https://github.com/attila-lendvai/quicklisp-client/tree/defsystem-depends-on-with-find-system-hook

and here's the nonintrusive variant: https://github.com/attila-lendvai/quicklisp-client/tree/defsystem-depends-on-fix

[attila-lendvai]

damn it! the hook variant is broken, i'll look at it tomorrow afternoon, eu time...

[attila-lendvai]

ok, here's the issue with the find-system hook:

component-loaded-p calls find-system, and thus ultimately calling (require :sb-cover) initiates the installation of cl-json.test, which is not something we want.

 18: (QUICKLISP-CLIENT::SYSTEM-DEFINITION-SEARCHER "cl-json.test")
 19: ((FLET ASDF/FIND-SYSTEM::TRY :IN ASDF/FIND-SYSTEM:SEARCH-FOR-SYSTEM-DEFINITION) QUICKLISP-CLIENT::SYSTEM-DEFINITION-SEARCHER)
 20: (SB-KERNEL:%MAP-FOR-EFFECT-ARITY-1 #<CLOSURE (FLET ASDF/FIND-SYSTEM::TRY :IN ASDF/FIND-SYSTEM:SEARCH-FOR-SYSTEM-DEFINITION) {1006FF31AB}> (SWANK::FIND-SWANK-ASDF-SYSTEM ASDF/PACKAGE-INFERRED-SYSTEM:SY..
 21: (ASDF/FIND-SYSTEM:SEARCH-FOR-SYSTEM-DEFINITION "cl-json.test")
 22: (ASDF/OPERATE:COMPONENT-LOADED-P "cl-json.test")
 28: (REMOVE-IF-NOT ASDF/OPERATE:COMPONENT-LOADED-P ("sb-bsd-sockets" "asdf/prelude" "asdf/driver" "asdf/defsystem" "asdf" "uiop" ...))
 29: (ASDF/OPERATE:REQUIRE-SYSTEM #<ASDF/OPERATE:REQUIRE-SYSTEM "sb-md5"> :VERBOSE NIL)
 30: (ASDF/OPERATE:MODULE-PROVIDE-ASDF :SB-MD5)
 31: ((FLET #:WRAPPER33 :IN REQUIRE) ASDF/OPERATE:MODULE-PROVIDE-ASDF)
 32: (SB-KERNEL:%MAP-FOR-EFFECT-ARITY-1 #<CLOSURE (FLET #:WRAPPER33 :IN REQUIRE) {7FFFF0A94BBB}> (ASDF/OPERATE:MODULE-PROVIDE-ASDF SB-IMPL::MODULE-PROVIDE-CONTRIB))
 33: (REQUIRE :SB-MD5 NIL)
 34: (SB-FASL::LOAD-FASL-GROUP #S(SB-FASL::FASL-INPUT :STREAM #<SB-SYS:FD-STREAM for "file /home/alendvai/common-lisp/sbcl/obj/sbcl-home/contrib/sb-cover.fasl" {1006869C93}> :TABLE #(4 PROVIDE :SB-COVER RE..
 35: (SB-FASL::LOAD-AS-FASL #<SB-SYS:FD-STREAM for "file /home/alendvai/common-lisp/sbcl/obj/sbcl-home/contrib/sb-cover.fasl" {1006869C93}> NIL NIL)
 36: ((FLET SB-FASL::LOAD-STREAM :IN LOAD) #<SB-SYS:FD-STREAM for "file /home/alendvai/common-lisp/sbcl/obj/sbcl-home/contrib/sb-cover.fasl" {1006869C93}> T)
 37: (LOAD #P"/home/alendvai/common-lisp/sbcl/obj/sbcl-home/contrib/sb-cover.fasl" :VERBOSE NIL :PRINT NIL :IF-DOES-NOT-EXIST T :EXTERNAL-FORMAT :DEFAULT)
 38: (SB-IMPL::MODULE-PROVIDE-CONTRIB "sb-cover")
 39: ((FLET #:WRAPPER33 :IN REQUIRE) SB-IMPL::MODULE-PROVIDE-CONTRIB)
 40: (SB-KERNEL:%MAP-FOR-EFFECT-ARITY-1 #<CLOSURE (FLET #:WRAPPER33 :IN REQUIRE) {7FFFF0A9522B}> (ASDF/OPERATE:MODULE-PROVIDE-ASDF SB-IMPL::MODULE-PROVIDE-CONTRIB))
 41: (REQUIRE "sb-cover" NIL)

the culprit is probably (already-loaded-systems) here:

(defun require-system (system &rest keys &key &allow-other-keys)
    (apply 'load-system system :force-not (already-loaded-systems) keys))

@fare is this an ASDF bug? or a misuse of the hook?

[fare]

So require-system asks whether registered systems are loaded. That should be OK as long as your find-system hook doesn't actually load a system, but only loads its .asd, as it probably should.

[fare]

So, I could have registered-systems return the system objects instead of their names, and that would save a trip through find-system and it hook.

Still, a find-system hook should not actually be loading the system (as per load-op or load-system), only loading its .asd.

[fare]

Try ASDF commit c0fca7ac66bbf510f68d00d8e030c98c11204439

[attila-lendvai]

a summary of the situation.

i have two proposed solutions as follows:

the hook variant

it is @fare's preferred way. it installs a hook into asdf:system-definition-search-functions. this hook does one thing only: for each missing system name it looks them up in the ql database and offers/tries to install them one by one, from inside the hook.

requires a very recent ASDF.

note that it does not load the systems, and thus in its current form it eliminates the fancy load-strategy code and the macroexpand output (i commented them out in the commit to mark this fact).

this commit is not ready as is, it's more of a proof of concept that needs to be cleaned up if @xach chooses this variant. i don't have the bird's eye view perspective on quicklisp, so the cleanup probably needs to be done by @xach, or i need guidance.

this variant is more flexible in that it can be enabled with a ql:with-automatic-installing wrapper, or by setting a global. while enabled it will initiate install regardless of who and why calls asdf:find-system, and does not interfere with error handling (unpatched ql also re-signals the errors, but it unwinds the stack).

https://github.com/attila-lendvai/quicklisp-client/tree/defsystem-depends-on-with-find-system-hook

the just-patch-it variant

this is a non-intrusive change that tries to identify the internal state of asdf and decide based on that. works as is, also with older ASDF's, and is ready to be merged.

the downside is that it relies on decoding ASDF's internal state from the signalled system not found error (as opposed to using a published API). as of today ASDF's error is not very clear about the situation, and the ql code doesn't make much sense without looking at ASDF's code.

https://github.com/attila-lendvai/quicklisp-client/tree/defsystem-depends-on-fix

[fare]

An automatic batching variant or the installer would have its defsystem return a proxy object, and an operate method that installs batched systems, change-class the proxy objects, and restarts the operate method. That's if batching is needed.

[attila-lendvai]

@fare, do you see anything effecting/simplifying this issue in the 3.3.0 release and its internal refactorings?

[attila-lendvai]

another possible solution: #128

[attila-lendvai]

as reported in #108, the original bug that motivated this PR has been fixed.

closing this and opening a new issue to discuss potentially installing/supporting asdf:*system-not-found-hooks*.