Would love some feedback on wip feature

[Stelian Ionescu]

My apologies for the delay in answering, but I was a bit busy last
quarter.

On Sun, 2016-08-21 at 23:41 +0200, Chris Bagley wrote:
> Hi folks,
> 
> I'm currently working on adding caching of .so and wrapper files to
> cffi. This is to handle issues arising from trying to use various
> libraries on machines without a C compiler. I'm hoping you can take a
> peek at my branch [0] and see if I'm going in the right direction.

You haven't stated what those issues are, not why you think this
approach is the best way to solve them nor why do you think it's
reasonable for a development machine to have a CL compiler but not a C
compiler.

> The changes are very simple, we allow users to specify a cache
> directory by using an optional :cache-dir argument in :grovel-file &
> :wrapper-file forms. CFFI will then look there first for the required
> files before trying to compile them itself.
> 
> The tricky bit is that we recommend that people use the #+ & #-
> reader
> conditionals in the grovel and wrapper specifications, which means
> the
> cached results are only applicable if all the required feature-forms
> match.
> 
> To handle this I am currently using my with-cached-reader
> -conditionals
> library [1] which, whilst small and standalone, could be replicated
> inside CFFI if required. The library modifies the readtable so that
> feature expression still work as before, but the also record the
> feature-forms used.
> 
> With the captured feature information we can make a subdirectory
> inside the :cache-dir directory, whose name is based on the feature
> information. In my current experiment it does something simple & easy
> to read, however it would likely make directory names too long for
> windows users.
> 
> For example I took osicat and added a cache directory to the
> following lines:
> 
>     (:grovel-file "basic-unixint" :cache-dir "foo")
>     (:wrapper-file "wrappers" :soname "libosicat" :cache-dir "foo")
> 
> The "foo" directory has the following contents
> 
>     bsd_nil_darwin_nil_freebsd_nil_linux_t_openbsd_nil
>         unixint.processed-grovel-file
>     darwin_nil_linux_t_mips_nil_openbsd_nil_windows_nil
>         basic-unixint.processed-grovel-file
>     linux_t_windows_nil
>         libosicat.so
>         wrappers.processed-wrapper-file
> 
> All the system does is copy these files to the same directory the
> system would have put the compiled results in the standard system.

So this patch

1) Duplicates ASDF functionality in incompatible ways - like the choice
of a mapping from source file name to build artifact name
2) Undermines ASDF's internal invariants by copying files into the ASDF
cache from a secondary cache and creates empty files in the ASDF cache
3) Creates cache keys using one of the most brittle mechanism in Common
Lisp (*features*).
Try thinking what happens during interactive development when a
developer changes *features* between compilations.
4) Makes things difficult for the creation of a binary package because
now that different developers can set different :cache-dir directories
in the .asd files they control, a user must review and possibly
manually edit all .asd files of its dependencies (think pgloader for
example) in order to enforce the use of the same cache directory and to
be able to find the build artifacts.

If at all, this code should be part of ASDF as a cache available for
all extension types. There are 2 important rules in doing cross-OS
development and deployment:

1) Follow the conventions of the host OS, don't fight them
2) Work with your build system

This patch goes against both principles, so as far as I am concerned my
vote is a NACK. I'm sorry if you've worked so long on this, but in my
opinion this feature should not be a part of CFFI.

-- 
Stelian Ionescu a.k.a. fe[nl]ix
Quidquid latine dictum sit, altum videtur.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part
URL: <https://mailman.common-lisp.net/pipermail/cffi-devel/attachments/20161004/bdd19cd4/attachment.sig>