CCL 1.6 breaks cl-fad (or vice versa)

[xach]

cl-fad defines and exports a symbol named DELETE-DIRECTORY from the CCL package. This is not a great idea on cl-fad's part, but as it stands, many projects are broken by upgrading to CCL 1.6.

See openmcl.lisp in the cl-fad distribution for the problematic code.

[xach]

Effects of cl-fad failure

[rme]

It's pretty hard to suppress my initial reaction of "cl-fad is clearly out of bounds messing with implementation package."

I appreciate that there are numerous libraries which use cl-fad, but our choices are pretty limited as far as I can see:

• leave out the new ccl:delete-directory entirely
• rename our new ccl:delete-directory to something else

The first choice is undesirable, I think: users have rightfully complained that ccl provided no way to delete directories.

The second choice is not attractive either: delete-directory is the obvious name. and I don't like giving cl-fad squatter's rights to it, especially since cl-fad is doing something (messing with the implementation package) it shouldn't be doing.

If there are other, more palatable, options, I'd be glad to hear them.

One could argue that "breaking" cl-fad will harm ccl's reputation, but the fact is that cl-fad is in the wrong. I'm inclined to stand on principle in this instance.

[gb]

It's not a good idea to define and export things from implementation packages for precisely this reason, right ? (Or, at the very least, the possibility that the implementation could someday offer a conflicting definition is one of the strongest arguments against that practice.)

(There may actually be a stronger argument against it: there was a case a few years ago where a user defined and exported something from the CCL package and their users started reporting bugs in that function to us. Yep. That's definitely even worse.)

I (too often) preface things by saying "I don't want to sound like a total jerk about this, but ..."; in this case, I personally find that sounding like a total jerk is entirely appropriate.

1. Don't do this.
2. If you do and things break, it's your responsibility to fix them.

I don't think that it's reasonable to expect implementors (jerks and otherwise) to survey all users to ensure that changes to implementation packages won't break code that defines things in those packages. I can't imagine that scaling well, and I think that there are additional issues involving "good names". DELETE-DIRECTORY is a very good name for a function that ... deletes directories; if the implementation can't use that name because it conflicts with third-party code, then it may have to use another name that's harder to remember or less suggestive of functionality or otherwise less desirable. (I get "REMOVE-DIRECORY" then stuff like "CAUSE-DELETION-OF-DIRECTORY"; I'm already scraping the bottom of the barrel here.)

I completely agree that CCL should have provided a reasonable, documented, exported DELETE-DIRECTORY in the first place, but I'm not telling you anything new in saying that working around its absence the way that you did stands a good chance of breaking. There may be less fragile workarounds that're somewhat less fragile (and only slightly uglier.)

;;; At least approximately ...
(eval-when (:compile-toplevel :execute)
  (unless (find-symbol "DELETE-DIRECTORY" "CCL")
    (pushnew :cl-fad-needs-to-define-delete-directory *features*)))

...

#+cl-fad-needs-to-define-delete-directory
(progn
(defun ccl::delete-directory (dir) ...)

;;; Ensure that the symbol's exported, too.
...
)

That only works if you know that "your" CCL:DELETE-DIRECTORY has the same signature as the one that may (eventually) be provided by the implementation. If you don't know that, it may be possible to minimize the chance of conflicts by intentionally choosing and using a bad name for the function.

If those approaches aren't practical, I think that the only things that you can do are:

• report the missing functionality to the implementors and advise your users that that implementation won't be supported until it provides that functionality.
• do what you did and expect breakage.

[xach]

I opened this ticket in the hopes that someone more familiar with CCL than me could suggest a better approach to Edi Weitz, the maintainer of cl-fad. Clearly, mucking around with the ccl package, as cl-fad does now, was never a great idea.

I don't normally use ClozureCL, so I'm not comfortable suggesting a fix. I don't think Edi uses CCL either (I don't think he wrote the original ccl support), so he might not be able to come up with the best fix on his own, either.

My best hope for this ticket is someone with CCL experience looking at cl-fad's openmcl.lisp, saying "Oh, here's a future-proof and backwards-compatible way to do it," and sending Edi a patch, which he promptly applies.

[xach]

I sent a patch to Edi and he says he'll try to make a new release in a few weeks.

[gb]

The error that one gets when trying to redefine a built-in function in CCL is continuable, so (at least) if the conflicting version is loaded interactively one can just (CONTINUE) from the break loop.

Another short-term workaround would be to

(fmakunbound 'ccl::delete-directory)

before loading the conflicting code. (Nothing in CCL itself currently calls DELETE-DIRECTORY, AFAIK; it's possible of course that tools used to load CL-FAD do so.)

The CERROR-on-redefinition can be suppressed by binding the doubly-misnamed special variable CCL:*WARN-IF-REDEFINE-KERNEL* to NIL. (It's doubly-misnamed because the function names that're protected from redefinition aren't in the kernel and because redefinition of protected functions causes a CERROR, not a warning.)

(let* ((ccl::*warn-if-redefine-kernel* nil))
  (whatever-one-does-to-load-cl-fad))

That's possibly a little dangerous in that it'd allow any built-in function to be quietly redefined; if it's known that the only attempted redefinition involves that of CCL::DELETE-DIRECTORY, then the risk is obviously low.

The condition signaled by redefinition is just a SIMPLE-ERROR (so it may be awkward to set up a handler for it), and there's no way to automatically allow redefinition of some functions but not others.

[xach]

Here's what I wound up sending to him:

diff --git a/fad.lisp b/fad.lisp
index e2fb22d..b99e5b8 100644
--- a/fad.lisp
+++ b/fad.lisp
@@ -272,7 +272,7 @@ DIRNAME does not exist."
                                                         file (unix:get-unix-error-msg errno))))
                                       #+:sbcl (sb-posix:rmdir file)
                                       #+:clisp (ext:delete-dir file)
-                                      #+:openmcl (ccl:delete-directory file)
+                                      #+:openmcl (cl-fad-ccl:delete-directory file)
                                       #+:cormanlisp (win32:delete-directory file)
                                       #+:ecl (si:rmdir file)
                                       #+(or :abcl :digitool) (delete-file file))
diff --git a/openmcl.lisp b/openmcl.lisp
index afecdac..0fc9185 100644
--- a/openmcl.lisp
+++ b/openmcl.lisp
@@ -27,22 +27,43 @@
 ;;; NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
 ;;; SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-(in-package :ccl)
+(in-package :cl-fad)
 
 (eval-when (:compile-toplevel :load-toplevel :execute)
-  (let ((%rmdir-symbol (find-symbol "%RMDIR" :ccl)))
-    (unless (and %rmdir-symbol (fboundp %rmdir-symbol))
-      (pushnew :no-%rmdir *features*))))
+  (flet ((ccl-function-feature (symbol-name feature)
+           (let ((symbol (find-symbol symbol-name :ccl)))
+             (when (and symbol (fboundp symbol))
+               (pushnew feature *features*)))))
+    (ccl-function-feature "%RMDIR" :ccl-has-%rmdir)
+    (ccl-function-feature "DELETE-DIRECTORY" :ccl-has-delete-directory)))
 

-#+:no-%rmdir
+(defpackage :cl-fad-ccl
+  (:use :cl)
+  (:export delete-directory)
+  (:import-from :ccl
+                :%realpath
+                :signal-file-error
+                :native-translated-namestring
+                :with-cstrs)
+  #+ccl-has-%rmdir
+  (:import-from :ccl :%rmdir)
+  #+ccl-has-delete-directory
+  (:import-from :ccl :delete-directory))
+
+(in-package :cl-fad-ccl)
+
+#-ccl-has-%rmdir
 (defun %rmdir (name)
   (with-cstrs ((n name))
     (#_rmdir n)))
 
+;;; ClozureCL 1.6 introduced ccl:delete-directory with semantics that
+;;; are acceptably similar to this "legacy" definition.
+
+#-ccl-has-delete-directory
 (defun delete-directory (path)
   (let* ((namestring (native-translated-namestring path)))
     (when (%realpath namestring)
       (let* ((err (%rmdir namestring)))
         (or (eql 0 err) (signal-file-error err path))))))
 
-(export 'delete-directory)

[xach]

Looks like Edi has applied my patch and cl-fad 0.6.4 works fine with CCL 1.6.