Re: Issues with package location information on SBCL
On Mon, Feb 19, 2018 at 7:03 PM, Eric Timmons wrote: > I've also opened the following bug on SBCL to let them know about it: > https://bugs.launchpad.net/sbcl/+bug/1750466. This bug has been fixed upstream and should probably appear in SBCL 1.4.7. Since they got that fixed before ASDF 3.3.2 got out, is it worth reverting commit 21e3a85? That would keep the return of `parse-define-package-form` unchanged and the warnings would only show up in SBCL 1.4.1-1.4.6 inclusive. -Eric
Re: Issues with package location information on SBCL
On Tue, Feb 20, 2018 at 3:35 PM, Robert Goldman wrote: > This does seem to illustrate an issue with the current "export everything > that's in UIOP" strategy. > > Should we consider changing this policy? > 1- UIOP reexports everything that individual subpackages of UIOP export. I believe that's a simple and good policy. If a helper must remain private, don't export it in the subpackage. 2- UIOP/PACKAGE is special, in that it is defined by defpackage (for bootstrap reasons), and that therefore to portably ensure upgradability, the list of symbols it exports MAY NEVER CHANGE, EVER. No adds, no deletes, no renames. No change. If you need any change, define and export a different package. Or use defpackage to define an empty package or constant package, then define-package to import-from and reexport from it. > Or, if we have "internal" functions that we don't want to be visible through > UIOP/DRIVER, should we simply not export them from the sub-packages and use > :import-from to move them among the sub-packages? > Yes, that's the idea so far. Also, we've moved symbols within UIOP in the past, and the UIOP/subpackage names aren't stable. If you're using a utility in UIOP, use UIOP: as a symbol prefix, don't use the symbol from its current subpackage. —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org Statism is the secular version of salvation through faith: it doesn't matter what bureaucrats do, only that they do it with good intentions.
Re: Issues with package location information on SBCL
This does seem to illustrate an issue with the current "export everything that's in UIOP" strategy. Should we consider changing this policy? Or, if we have "internal" functions that we don't want to be visible through `UIOP/DRIVER`, should we simply not export them from the sub-packages and use `:import-from` to move them among the sub-packages? Best, r On 19 Feb 2018, at 18:22, Faré wrote: On Mon, Feb 19, 2018 at 7:03 PM, Eric Timmons wrote: Glad to help! I've also opened the following bug on SBCL to let them know about it: https://bugs.launchpad.net/sbcl/+bug/1750466. Thanks! Also, I checked that nothing else in ASDF uses `parse-define-package-form`, but I somehow missed until just now that it's actually exported from uiop =/. Is there any concern about another library using it? If so, I can try to fix it in such a way that the output of `parse-define-package-form` is unchanged. I wouldn't worry about that: grepping through quicklisp systems reveals no user. —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org It costs the nation millions to keep Gandhi living in poverty — Sarojini Naidu
Re: Issues with package location information on SBCL
On Mon, Feb 19, 2018 at 7:03 PM, Eric Timmons wrote: > Glad to help! I've also opened the following bug on SBCL to let them know > about it: https://bugs.launchpad.net/sbcl/+bug/1750466. > Thanks! > Also, I checked that nothing else in ASDF uses `parse-define-package-form`, > but I somehow missed until just now that it's actually exported from uiop > =/. Is there any concern about another library using it? If so, I can try to > fix it in such a way that the output of `parse-define-package-form` is > unchanged. > I wouldn't worry about that: grepping through quicklisp systems reveals no user. —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org It costs the nation millions to keep Gandhi living in poverty — Sarojini Naidu
Re: Issues with package location information on SBCL
Thanks a lot for fixing this issue! I opened a MR based on it: https://gitlab.common-lisp.net/asdf/asdf/merge_requests/92 —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org To be an anarchist only means that you believe that aggression is not justified, and that states necessarily employ aggression. And, therefore, that states, and the aggression they necessarily employ, are unjustified. It's quite simple, really. It's an ethical view, so no surprise it confuses utilitarians. —- N. Stephan Kinsella On Sat, Feb 17, 2018 at 12:30 AM, Eric Timmons wrote: > 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
Issues with package location information on SBCL
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 &rest 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 &rest clauses) "DEFINE-PACKAGE takes a PACKAGE and a number of CLAUSES, of the form @@ -733,7 +733,7 @@ (defmacro define-package (package &rest clauses) UNINTERN -- Remove symbols here from PACKAGE." (let ((ensure-form `(prog1 - (apply 'ensure-package ',(parse-define-package-form package clauses)) + (funcal