Issues with package location information on SBCL

[Eric Timmons]

I've started testing ASDF 3.3.1.3 with my group's code on SBCL 1.4.4
and noticed some issues with uiop:define-package (due to commit
8281e011).

First, when compiling ASDF I get 456 compilation notes
(https://pastebin.com/NnRUKGWe). I get the same notes when using
uiop:define-package in our code as well. I honestly think this is an
issue of SBCL being over aggressive. It's also odd because if the
recording of the source location is removed, then the notes aren't
produced. It appears this started happening in SBCL 1.4.1 (potentially
due to commit af5e2ed1e).

The volume of notes (especially when using package-inferred-system)
can drown out real issues. I'll likely report this as an issue to SBCL
since the keywords *should* be constant and the source transform for
apply doesn't seem to be preserving that. However, it could also be
fixed in ASDF by changing the apply to a funcall in the define-package
expansion (patch 2).

Second, the comma on (sb-c:source-location) seems to cause the source
location for the package to always point to the body of the
define-package macro. Patch 1 changes this so that form is evaluated
after the macro is expanded. I didn't test this on anything earlier
than SBCL 1.4.4, but I don't believe this behavior is version
dependent.

-Eric
From 79baf14021d8940ef2a969359fa335e72d8fad57 Mon Sep 17 00:00:00 2001
From: Eric Timmons 
Date: Fri, 16 Feb 2018 23:59:49 -0500
Subject: [PATCH 1/2] Evaluate sb-c:source-location after macroexpansion

Tested on SBCL 1.4.4. If sb-c:source-location is evaluated during macro
expansion, then the source location will always point to asdf.lisp (inside the
define-package macro). If it is evaluated after macro expansion, it points to
the right place.
---
 uiop/package.lisp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/uiop/package.lisp b/uiop/package.lisp
index 4a6d38e1..97981752 100644
--- a/uiop/package.lisp
+++ b/uiop/package.lisp
@@ -735,7 +735,7 @@ (defmacro define-package (package  clauses)
  `(prog1
   (apply 'ensure-package ',(parse-define-package-form package clauses))
 #+sbcl (setf (sb-impl::package-source-location (find-package ',package))
- ,(sb-c:source-location)
+ (sb-c:source-location)
 `(progn
#+(or clasp ecl gcl mkcl) (defpackage ,package (:use))
(eval-when (:compile-toplevel :load-toplevel :execute)
-- 
2.16.1

From 8d887763cab34b32f2aca0e0a655c3017f80be9c Mon Sep 17 00:00:00 2001
From: Eric Timmons 
Date: Sat, 17 Feb 2018 00:04:42 -0500
Subject: [PATCH 2/2] Change apply to funcall in expansion of define-package

SBCL has been getting aggresive with checking arguments to functions. For some
reason (as of SBCL 1.4.1), setting the package source location in define-package
seems to trigger a source translation for the apply form that ends up producing
(many) compilation notes that the arguments to ensure-package in the keyword
positions are not constant, weakening keyword argument checking. We can get
around that, however, by using a funcall directly.
---
 uiop/package.lisp | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/uiop/package.lisp b/uiop/package.lisp
index 97981752..c2dd14da 100644
--- a/uiop/package.lisp
+++ b/uiop/package.lisp
@@ -700,13 +700,13 @@ (defun parse-define-package-form (package clauses)
 :and :do (setf use-p t) :else
   :when (eq kw :unintern) :append args :into unintern :else
 :do (error "unrecognized define-package keyword ~S" kw)
-  :finally (return `(,package
- :nicknames ,nicknames :documentation ,documentation
- :use ,(if use-p use '(:common-lisp))
- :shadow ,shadow :shadowing-import-from ,shadowing-import-from
- :import-from ,import-from :export ,export :intern ,intern
- :recycle ,(if recycle-p recycle (cons package nicknames))
- :mix ,mix :reexport ,reexport :unintern ,unintern)
+  :finally (return `(',package
+ :nicknames ',nicknames :documentation ',documentation
+ :use ',(if use-p use '(:common-lisp))
+ :shadow ',shadow :shadowing-import-from ',shadowing-import-from
+ :import-from ',import-from :export ',export :intern ',intern
+ :recycle ',(if recycle-p recycle (cons package nicknames))
+ :mix ',mix :reexport ',reexport :unintern ',unintern)
 
 (defmacro define-package (package  clauses)
   "DEFINE-PACKAGE takes a PACKAGE and a number of CLAUSES, of the form
@@ -733,7 +733,7 @@ (defmacro define-package (package  clauses)
 UNINTERN -- Remove symbols here from PACKAGE."
   (let ((ensure-form
  `(prog1
-  (apply 'ensure-package ',(parse-define-package-form package clauses))

Re: Testing for ASDF 3.3.2 and beyond?

[Faré]

Anton,

thanks a lot for the test! asdf 3.3.1.3 looks good to go.

Robert,

I had no trouble loading clws or lime with clisp, cl-rrt or
cl-sat.minisat.test or inner-conditional-test with sbcl (using asdf
3.3.1.3 from master).

The ecl failures I could reproduce, but I'm not worried, as
lisp-executable is not supported with recent asdf, and a reproducible
out-of-memory error on one system :trivia.balland2006.enabled.test
isn't outrageous.

CL_SOURCE_REGISTRY='(:source-registry
:ignore-inherited-configuration)' rlwrap clisp -I -x "(and '(#.(load
\"/home/fare/cl/asdf/build/asdf.lisp\") #.(in-package :asdf)
#.(upgrade-asdf) #.(load \"~/quicklisp/setup.lisp\") #.(ql:quickload
:clws)) ())"
CL_SOURCE_REGISTRY='(:source-registry
:ignore-inherited-configuration)' rlwrap clisp -I -x "(and '(#.(load
\"/home/fare/cl/asdf/build/asdf.lisp\") #.(in-package :asdf)
#.(upgrade-asdf) #.(load \"~/quicklisp/setup.lisp\") #.(ql:quickload
:lime)) ())"

CL_SOURCE_REGISTRY='(:source-registry
:ignore-inherited-configuration)' rlwrap sbcl --eval "(and '(#.(load
\"/home/fare/cl/asdf/build/asdf.lisp\") #.(in-package :asdf)
#.(upgrade-asdf) #.(load \"~/quicklisp/setup.lisp\") #.(ql:quickload
:cl-rrt)) ())"
CL_SOURCE_REGISTRY='(:source-registry
:ignore-inherited-configuration)' rlwrap sbcl --eval "(and '(#.(load
\"/home/fare/cl/asdf/build/asdf.lisp\") #.(in-package :asdf)
#.(upgrade-asdf) #.(load \"~/quicklisp/setup.lisp\") #.(ql:quickload
:cl-sat.minisat.test )) ())"
CL_SOURCE_REGISTRY='(:source-registry
:ignore-inherited-configuration)' rlwrap sbcl --eval "(and '(#.(load
\"/home/fare/cl/asdf/build/asdf.lisp\") #.(in-package :asdf)
#.(upgrade-asdf) #.(load \"~/quicklisp/setup.lisp\") #.(ql:quickload
:inner-conditional-test )) ())"

CL_SOURCE_REGISTRY='(:source-registry
:ignore-inherited-configuration)' rlwrap ecl --eval "'(#.(load
\"/home/fare/cl/asdf/build/asdf.lisp\") #.(in-package :asdf)
#.(upgrade-asdf) #.(load \"~/quicklisp/setup.lisp\") #.(ql:quickload
:lisp-executable-example)#.(quit))"
CL_SOURCE_REGISTRY='(:source-registry
:ignore-inherited-configuration)' rlwrap ecl --eval "'(#.(load
\"/home/fare/cl/asdf/build/asdf.lisp\") #.(in-package :asdf)
#.(upgrade-asdf) #.(load \"~/quicklisp/setup.lisp\") #.(ql:quickload
:trivia.balland2006.enabled.test )#.(quit))"

Often failures in cl-test-grid are "just" the result of using too
little memory, or batching system loads, or some other reason, and
have to be retried.

I can try to activate that Windows VM and run the all the tests there,
if you want...

—♯ƒ • François-René ÐVB Rideau •Reflection• http://fare.tunes.org
Pain is inevitable. Suffering is optional.



On Fri, Feb 16, 2018 at 11:42 AM, Robert Goldman  wrote:
> Thanks for sending that.  Had a quick look.  One nice thing is that there's
> a very limited number of regressions.  I'll look at those when I can.
>
> Faré: I didn't believe it was possible to downgrade ASDF, but we see this
> here in a couple of cases for ECL.  ECL is trying to reload "prebuilt-asdf".
> I think we can ignore these failures on ECL. They just should not do that,
> and it's not really and ASDF test failure if they load a conflicting version
> of ASDF, breaking our upgrade method.
>
> clisp I refuse to care about, since it's effectively abandonware, unless you
> are willing to build from source, which I am not.  Certainly clisp 2.49 is
> abandonware.k
>
> That leaves only the SBCL and ACL failures for us to worry about  I'm
> pretty busy this weekend, but I will have a look.
>
> Best,
> Robert
>
>
> On 15 Feb 2018, at 22:11, Anton Vodonosov wrote:
>
>> Robert, what delay are you apologizing for? I'm aware only of the delay
>> from my side. :)
>>
>>
>>
>> The results for ASDF 3.3.1.3:
>> 
>>
>> Lisps tested so far:
>>
>>
>>
>> abcl-1.5.0-fasl43-linux-x86
>>
>> acl-10.0s-linux-x86
>>
>> ccl-1.11-r16635-f96-linux-x86
>>
>> clisp-2.49-unix-x86
>>
>> ecl-16.1.2-unknown-linux-x86-bytecode
>>
>> ecl-16.1.2-unknown-linux-x86-lisp-to-c
>>
>> sbcl-1.3.21-linux-x86
>>
>>
>>
>> Best regards,
>>
>> \- Anton
>>
>>
>>
>>
>>
>> 14.02.2018, 22:02, "Robert Goldman" :
>>
>>> OK, as I said, sorry about the delay. Anton, in place of Fare's #3 below,
>>> will you please just test what's in the
>>> `syntax-control-based-on-standard-syntax` branch? The comparison between 2
>>> and 3 will tell us to what extent it's an issue to lock in standard syntax
>>> instead of whatever happens to be the current readtable when ASDF is loaded.
>>>
>>> Thanks!
>>>  r
>>>
>>> On 13 Feb 2018, at 22:36, Robert P. Goldman wrote:
>>>
 I'll get you a branch with the other setting for the syntax control, so
 you can just test with that instead of having to modify anything yourself.
 I'll get it pushed sometime tomorrow.
>>>
>>>
>>>  Sorry for the delay.
>>>
>>>  Sent from my iPad


> On Feb 13, 2018, at 20:15, Anton Vodonosov
> 

Re: Testing for ASDF 3.3.2 and beyond?

[Robert Goldman]

Thanks for sending that.  Had a quick look.  One nice thing is that 
there's a very limited number of regressions.  I'll look at those when I 
can.


Faré: I didn't believe it was possible to downgrade ASDF, but we see 
this here in a couple of cases for ECL.  ECL is trying to reload 
"prebuilt-asdf".  I think we can ignore these failures on ECL. They just 
should not do that, and it's not really and ASDF test failure if they 
load a conflicting version of ASDF, breaking our upgrade method.


clisp I refuse to care about, since it's effectively abandonware, unless 
you are willing to build from source, which I am not.  Certainly clisp 
2.49 is abandonware.k


That leaves only the SBCL and ACL failures for us to worry about  
I'm pretty busy this weekend, but I will have a look.


Best,
Robert


On 15 Feb 2018, at 22:11, Anton Vodonosov wrote:

Robert, what delay are you apologizing for? I'm aware only of the 
delay from my side. :)




The results for ASDF 3.3.1.3: 



Lisps tested so far:



abcl-1.5.0-fasl43-linux-x86

acl-10.0s-linux-x86

ccl-1.11-r16635-f96-linux-x86

clisp-2.49-unix-x86

ecl-16.1.2-unknown-linux-x86-bytecode

ecl-16.1.2-unknown-linux-x86-lisp-to-c

sbcl-1.3.21-linux-x86



Best regards,

\- Anton





14.02.2018, 22:02, "Robert Goldman" :

OK, as I said, sorry about the delay. Anton, in place of Fare's #3 
below, will you please just test what's in the 
`syntax-control-based-on-standard-syntax` branch? The comparison 
between 2 and 3 will tell us to what extent it's an issue to lock in 
standard syntax instead of whatever happens to be the current 
readtable when ASDF is loaded.


Thanks!
 r

On 13 Feb 2018, at 22:36, Robert P. Goldman wrote:

I'll get you a branch with the other setting for the syntax control, 
so you can just test with that instead of having to modify anything 
yourself. I'll get it pushed sometime tomorrow.


 Sorry for the delay.

 Sent from my iPad


On Feb 13, 2018, at 20:15, Anton Vodonosov 
<[avodono...@yandex.ru]()> wrote:


 Faré, hello.

 Sorry for replying so long - I'm almost paralyzed by too many things 
I need to deal with currently.
 I've started tests for the following commit. Will follow-up with the 
results.


 commit 2a5bc3bece8f97fdf64dc73a4e0544a55ae38f9d
 Author: Robert P. Goldman 
<[rpgold...@sift.net]()>

 Date: Tue Jan 16 16:20:15 2018 -0600

 Bump version to 3.3.1.3



 02.02.2018, 23:06, "Faré" 
<[fah...@gmail.com]()>:



Dear Anton,


 can you run the below tests, in order or priority?

 1- Can you test what is currently in master, a.k.a. 3.3.1.3, as a
 release candidate for 3.3.2? It has been too long since 3.3.1 was
 released with several bugs that have impacted Quicklisp users.

 2- Can you test what is currently in the syntax-control branch as a
 release candidate for 3.3.3 or 3.4.0? We want to merge syntax 
control,
 but it's a significant enough change that we don't want it at the 
same

 time as the bug fixes. Also...

 3- Can you test what is currently in the syntax-control branch as a
 release candidate for 3.3.3 or 3.4.0, but with the following 
addition
 just after you load asdf but before you start using it: 
(defparameter
 uiop:*shared-readtable* (copy-readtable nil)) ? Indeed, we want to 
see

 what breaks if we disable extensions implementation-specific reader
 extensions. Test most important on CCL. I don't expect much breakage
 on other implementations, but it may exist, too.

 4- While you're at it, can you also run the test, at least on sbcl,
 with (defparameter uiop:*shared-readtable* 
uiop:*standard-readtable*))

 ? This will detect what breaks when we make the default readtable
 read-only.

 The thing is, we really want to have this syntax control, but we 
also

 want to preserve backward compatibility, and we can't make asdf
 stricter until every client is fixed (famous last word; of course we
 failed at exactly that in 3.3.1 — we could build correctly, but 
would

 also spuriously rebuild if secondary systems were misnamed; fixed in
 3.3.1.3).

 

 
 

 —♯ƒ • François-René ÐVB Rideau 
•Reflection• 
[http://fare.tunes.org]()
 A friend once asked me if I had ever considered terrorism as a means 
for
 political change. I replied that yes, I had considered it... and 
rejected it.

 Because it only causes change for the worse.
 Killing innocent people does not promote a culture of peaceful 
interaction.