search

Shen-Language / shen-cl

Shen for Common Lisp (Unmaintained)
BSD 3-Clause "New" or "Revised" License
122 stars 11 forks source link

Reintroduce backend.shen #39

Closed tizoc closed 5 years ago

tizoc commented 5 years ago
rkoeninger commented 5 years ago

Would it cause any problems to put this in the shen-cl package instead?

tizoc commented 5 years ago

Saw your comment and had to look at the original: https://github.com/Shen-Language/shen-sbcl/blob/master/backend.lsp#L70-L73

But the .shen file does return true: https://github.com/Shen-Language/shen-sbcl/blob/master/backend.shen#L106-L109

I'm making it return true because to me it makes no sense for it to return false

rkoeninger commented 5 years ago

Also, there was previously a platform.shen file for interop that could probably be restored as well.

tizoc commented 5 years ago

@rkoeninger re: shen-cl package, I guess it is the same, we can change it.

The platform.shen file is not needed anymore because some time ago I made the compiler do that work: https://github.com/Shen-Language/shen-cl/pull/15

rkoeninger commented 5 years ago

Oh, right package.lsp is something else.

rkoeninger commented 5 years ago

Re: checking in backend.lsp - I usually try to exclude any generated code; on the premise that source code repos should contain source code only.

tizoc commented 5 years ago

@rkoeninger same, but it is either the extra file in the repository or the extra dependency.

But I'm thinking the case here is differently, we already have a backend.lsp that is not generated code (well, it was initially, but then got a facelift and modifications). To compile the last version of something like backend.shen you just need a working Shen that can compile it, it doesn't have to be the last version. So instead of keeping a backend.lsp that is generated from the latest backend.lsp, we repurpose what is there and working now and use it just for bootstraping (so backend.lsp -> backend-bootstrap.lsp).

Some compilers go further and include boot images in the repository, but I think that would be the worst choice.

rkoeninger commented 5 years ago

More precisely, the choice is between a development+build dependency or a development dependency. You're always going to need shen.exe to work on the code, but not including backend.lsp would require shen.exe to build a fresh checkout as well. Developer just has to remember not to update backend.lsp directly.

Retaining backend-bootstrap.lsp is strange to me, you'd have to remember to update occasionally depending on changes that are made that would keep the old version from working (and know which those are).

Agree on last point, definitely don't want to include binaries in the repo.

tizoc commented 5 years ago

Retaining backend-bootstrap.lsp is strange to me, you'd have to remember to update occasionally depending on changes that are made that would keep the old version from working (and know which those are).

Only if you find a bug on it, or the kernel happens to change in a way that breaks it, which is unlikely. I'm still compiling Shen/Scheme with 0.18 (latest version is 0.22), and only because 0.18 is the earliest version having binaries on all platforms. Before that I always used a quite old Shen/SBCL version to compile it.

tizoc commented 5 years ago

@rkoeninger btw, what do you suggest? I'm not sure I understand what it is that you consider is the most convenient approach.

rkoeninger commented 5 years ago

Sorry, my vote is for no generated code or external dependencies in the repo. Then the repo has scripts or a build system to pull in the dependencies. At worst, there are instructions in the readme for how to get the dependencies you need.

tizoc commented 5 years ago

That sounds like what I was doing originally (which is what I do in Shen/Scheme).

I got mixed up because of this comment and though you were considering binaries from earlier releases as external dependencies, but re-reading that comment, you were talking only about the complexity of the dependency graph, not about external dependencies.

backend-bootstrap.lsp is not a generated file anymore than the current backend.lsp being that it is the same file. It saves the user from having to download the 10MB Shen binary, which doesn't exist on every platform that Shen/CL is supposed to support.

tizoc commented 5 years ago

I wonder if it is possible to avoid messing with the read-table and just use Common Lisp packages to differentiate between the Shen|KLambda and Common Lisp namespaces. Probably not going to be part of this PR, but something to consider for the future.

rkoeninger commented 5 years ago

just use Common Lisp packages to differentiate between the Shen|KLambda and Common Lisp namespaces

Something like this was attempted on a PR by @Drainful earlier in the year (or last year?). Can't get around some of the casing issues because you have to have case-sensitivity on and IF is the true name for if in CL. On that PR we talked about having case sensitivity on for one package (:SHEN) and off for another (:CL) but I don't think that's possible.

I remembered there's an aborted branch sitting around with some cleanup that you appear to be doing right now: https://github.com/Shen-Language/shen-cl/blob/ecl-enhancements/src/backend.lisp . It also puts all the backend functions in the shen-cl. prefix; renames and re-arranging things.

Also, I think the comments in backend.lsp that show the Shen equivalent can be removed maybe? They were just a reminder of what the Shen code looked like, but now that code will still be checked in the repo.

tizoc commented 5 years ago

I see, but if the read-table is left as-is, both IF and if will work, right?

What I had in mind was for Common Lisp code to be generated in lowercase, and the lisp. prefix is what tells the compiler that it is referencing something from the Common Lisp side (the compiler would do the same thing it does now, remove the prefix, etc, just skip the uppercase step).

Then the rest of the symbols get qualified with shen: in the translation from Klambda to Common Lisp.

The advantage is that now you can write Common Lisp like you normally would, and calling Shen from Common Lisp is easy too (it is just a bunch of functions in another namespace, no read-table shenanigans involved).

I do something similar in Shen/Scheme, by prefixing everything that comes from Klambda with kl:, but that just changes the symbol, I don't use namespaces. The Common Lisp version (if it works) would be cleaner in that respect.

I'm not that immersed into Common Lisp, so maybe I'm missing an important detail that makes this impossible, but if not I will open an issue and we can move the discussion there, I think it is something to take care of only after this PR has been solved because it is probably a bit more complicated than I'm thinking.

tizoc commented 5 years ago

Also, I think the comments in backend.lsp that show the Shen equivalent can be removed maybe? They were just a reminder of what the Shen code looked like, but now that code will still be checked in the repo.

I would rather leave that in, it helps me a lot when reading that code, so I'm very glad you added those comments back then.

tizoc commented 5 years ago

For some reason I haven't been able to figure out yet, shen.sysfunc? is returning true for everything, so for now I can launch the REPL with the compiled backend but I cannot define functions.

tizoc commented 5 years ago

What is weirder is that the individual components of the overwritten shen.sysfunc? version return false:

(12-) (shen.sysfunc? is-not-sysfunc)
true

(13-) (lisp. "(APPLY shen-cl.kernel-sysfunc? (LIST (QUOTE is-not-sysfunc)))")
false

(14-) (shen.lisp-prefixed? is-not-sysfunc)
false

hmmm....

(16-) (lisp. "(IF (shen.lisp-prefixed? 'is-not-sysfunc) 'true 'false)")
true

Ok, I figured it out, since lisp-prefixed? now is a shen functions it returns true/false and not common lisp booleans, both of which are true, mystery solved.

rkoeninger commented 5 years ago

true/false and not common lisp booleans, both of which are true

I love it when that happens. The Shen<->Host-language boolean situation is always weird.

This returns Shen true in shen-cl: (= () (lisp.= 0 1)).

tizoc commented 5 years ago

Yes, I hate that, but sadly, with Common Lisp using the empty list for "false" I don't think we can do any better.

tizoc commented 5 years ago

I just noticed that we don't do source releases for Shen/CL (that is, source + generated files). Doing so solves the bootstrapping problem without having to download a binary (which will not work for platforms for which no binary is published) or keeping backend.lsp in the repository.

tizoc commented 5 years ago

@rkoeninger here is what I did:

I think at least one release is needed using this setup, because with the namespace changes (shen -> shen-cl), plus some other tweaks, the code generated by previous releases of Shen/CL doesn't work. Future releases can just use binaries from this one release from now on (or download the sources and use backend.lsp from those sources as backend-bootstrap.lsp).

In the longer term it would be good to make backend.shen be compilable by any Shen (right now it has to be compiled by Shen/CL because it uses extensions). One possibility is porting Shen/Scheme's compiler so that it compiles to Common Lisp, it is compilable by any Shen, and also comes with some optimizations that would probably work with Common Lisp too.

rkoeninger commented 5 years ago

Getting a source release in there is a big win - shen-cl can be distributed as a quicklisp, embedded in other common lisp code bases, etc.

tizoc commented 5 years ago

I haven't checked any of those things, not being a Common Lisp user I'm not very familiar with the ecosystem, but good to know.

I think this is almost ready for merging, will give it another pass tomorrow to confirm it is all good before merging. If you see anything missing or think that there is a better way to do something let me know.

tizoc commented 5 years ago

Ok, I think this one is good to go, merging.

rkoeninger commented 5 years ago

RE: "Bailout from bootstrap if backend.lsp exists" I feel like bootstrap should build or rebuild the backend every time it's run. I want it build a new version, that's why I'm running it.

...whoops I wrote this comment but forgot to hit "Comment"

tizoc commented 5 years ago

@rkoeninger agree, but after spending some time trying to figure out how to do it portably in the Makefile I took the lazy approach and did it that way instead. Open to improvements.