Here is, as previously mentioned, the git format-patch with the modifications needed to port current ASDF head to MKCL 1.1.8 and later. This has been tested on Linux Ubuntu 12.04 and MKCL 1.1.8 where it passes all 51 tests of the current ASDF test-lisp without any failure.
Cheers,
Jean-Claude Beaudoin
On Sat, Mar 15, 2014 at 9:09 PM, Jean-Claude Beaudoin jean.claude.beaudoin@gmail.com wrote:
Here is, as previously mentioned, the git format-patch with the modifications needed to port current ASDF head to MKCL 1.1.8 and later. This has been tested on Linux Ubuntu 12.04 and MKCL 1.1.8 where it passes all 51 tests of the current ASDF test-lisp without any failure.
Hi, Jean-Claude,
thanks a lot for your patch. Congratulations! It mostly looks great. However, here some issues. Can you fix them and resubmit? Since only you can test with MKCL, I'd rather you do it.
1- Why the &allow-other-keys in + (defmethod initialize-instance :before ((o operation) &key &allow-other-keys) + (defmethod initialize-instance :before ((o non-propagating-operation) &key &allow-other-keys) + (defmethod reinitialize-instance :after ((self wild-module) &key &allow-other-keys) + (defmethod shared-initialize :after ((o operation) (slot-names t) &key &allow-other-keys) (defmethod initialize-instance :after ((plan filtered-sequential-plan)
That looks like a big bug in MKCL if you require that flag: it makes the function accept all keys, when if should only accept keys that are explicitly defined in *one of the methods*. Please fix MKCL.
2- I explicitly recommend against putting slots in operation. This is not well-supported by ASDF, and the constraints are not well-documented and subject to change in the future. Basically, the initargs may or may not be passed on to children operations, and are not taken in consideration by the table that remembers whether an action was done. Big no no. Instead, please add slots to system if you must, or just use new operations. The existing slots are there for backward-compatibility only, and I'd like to get rid of them.
3- No, it's NOT OK to rename bundle-system to bundled-system. That's not backward-compatible, and such a change requires maintainer approval. The maintainer is Robert.
4- Please follow the usual CL style for parens. No C-style dangling parens, please.
5- For the sake of ABCL, the test for mingw32 mingw64 should be at runtime, using (if (featurep '(:or :mingw32 ...)) or maybe adding that to the os-unix-p conditional branch.
6- How come you don't need the uiop-library-file on MKCL? Sounds dubious.
7- Please no #-(and) anywhere
8- in run-program, this looks very bad on non-mkcl implementations: #+os-windows + #+mkcl (list "cmd" '#:/c command)
9- Can you run the tests with one other implementation of your choice before you send, just to check you haven't broken anything obvious as above?
10- Also, you can remove this comment: + ;; both combined
PS: I saw and fixed slight bug in load-fasl-op, please update to the latest ASDF.
Robert, can you handle the merge when JCB comes with a cleaned up patch?
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org They laughed at Columbus, they laughed at Fulton, they laughed at the Wright brothers. But they also laughed at Bozo the Clown. — Carl Sagan
On Mar 15, 2014, at 22:12, Faré fahree@gmail.com wrote:
Robert, can you handle the merge when JCB comes with a cleaned up patch?
Sure. And I will run the tests. Unfortunately, my Linux test environment has not been restored (indeed, it's now just sitting in a box since our office move). So it would be nice if you could test, as well, to complement my Mac-only tests. Dave, if you could test on windows, that would be great, too.
On Sun, Mar 16, 2014 at 9:27 AM, Robert P. Goldman rpgoldman@sift.infowrote:
On Mar 15, 2014, at 22:12, Faré fahree@gmail.com wrote:
Robert, can you handle the merge when JCB comes with a cleaned up patch?
Sure. And I will run the tests. Unfortunately, my Linux test environment has not been restored (indeed, it's now just sitting in a box since our office move). So it would be nice if you could test, as well, to complement my Mac-only tests. Dave, if you could test on windows, that would be great, too.
I am currently working on the patch clean-up and also testing it on Win32 (XP) (where only one test still fails: monolithic-dll-op in test-bundle.script). Should be done shortly...
Regards,
Jean-Claude Beaudoin
On Sun, Mar 16, 2014 at 12:11 PM, Robert P. Goldman rpgoldman@sift.infowrote:
Thank you very much! I look forward to merging and testing.
Here is attached the cleaned-up patch in git format-patch. It has been tested with MKCL's current maintenance head (future 1.1.9) on Ubuntu/Linux 12.04 x86_64 and on Windows XP i386, both complete "make test-lisp" without failure. It has also been tested with sbcl 1.0.55.0.debian on Ubuntu 12.04.
Regards, Jean-Claude Beaudoin
On Mon, Mar 17, 2014 at 12:02 PM, Jean-Claude Beaudoin < jean.claude.beaudoin@gmail.com> wrote:
On Sun, Mar 16, 2014 at 12:11 PM, Robert P. Goldman rpgoldman@sift.infowrote:
Thank you very much! I look forward to merging and testing.
Here is attached the cleaned-up patch in git format-patch. It has been tested with MKCL's current maintenance head (future 1.1.9) on Ubuntu/Linux 12.04 x86_64 and on Windows XP i386, both complete "make test-lisp" without failure. It has also been tested with sbcl 1.0.55.0.debian on Ubuntu 12.04.
Regards, Jean-Claude Beaudoin
I just realized that the name of the attached file of my previous email is wrong (the content is OK). It reads "0001-Port-MKCL-3.1.0-to-MKCL-1.1.9.patch" but should be "0001-Port-ASDF-3.1.0-to-MKCL-1.1.9.patch" instead.
Random minor question:
In bundle.lisp why the replacement of 'defsystem with 'asdf/defsystem:defsystem? Did something go wrong for you with the former?
Other than that, I didn't see anything in the diff.
Passes all the tests on Mac OSX.
I'm inclined to merge the topic branch with master.
Looks great. A few minor nits and a larger issue.
Minor nits: * dangling ( at end of line, not in style * in the initialize-instance, I would still catch the :lisp-files and assert that it's null. That's protecting against people using it outside ECL (even on ECL, I'd like to get rid of it, but oh well — if people need something like that, the right thing is declare a component that has the proper input and/or output files. * in argv0, remove mkcl from comment about unsupported lisps. * you seem to have uncovered a bug in the untested prebuilt-system functionality it that is not for mkcl only. I tried to fix those bugs, please see 3.1.0.96. Also includes fix to image-op and a test of image-op in test-program.script. If you care about prebuilt-system, can you add tests for it in test-bundle and/or test-program?
Major issue: I think the way you special-case the hello-world example is defeating part of the point of the exercise. The point is that there should be a portable way to build executables on *all* supported lisps with the same defsystem, automatically calling the entry-point, etc.
That's why on ECL I need this uiop-library-file, to make sure UIOP is loaded, so we can call restore-image. That's also why create-image does the epilogue processing for you. I actually encourage you to reuse the create-image abstraction as is on MKCL, replacing the apply 'c::builder kind by an apply 'c::build-program and/or an apply (ecase kind ...) or an (ecase kind ...).
You need to download the lisp-invocation library. It includes an entry for MKCL that is copied over from ECL; if things have diverged, please send me a fix. While we're at it, please test cl-launch too if you have time: in the cl-launch repository, run ./cl-launch.sh -l mkcl -B tests.
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org The most urgent, is not that Government should teach, but that it should let teach. All monopolies are despicable, but the worst of them all, is the monopoly on education. — F. Bastiat
-
Faré -
Jean-Claude Beaudoin -
Robert P. Goldman