lokke-org / lokke

Lokke: Clojure for Guile
Other
199 stars 11 forks source link

initial macOS support #10

Closed aconchillo closed 3 years ago

aconchillo commented 3 years ago

Initial macOS support with as minimum changes as I could.

aconchillo commented 3 years ago

@rlbdv Two tests are failing in macos-guile-3

ERROR: test/example-nbody - too few tests run (expected 1, got 0)
ERROR: test/example-nbody - exited with status 2

Almost there. I'll try to figure out what's going on with guile 2.

aconchillo commented 3 years ago

guile 2.2 fixed. now these tests are failing in both:

ERROR: test/example-nbody - too few tests run (expected 1, got 0)
ERROR: test/example-nbody - exited with status 2
rlbdv commented 3 years ago

Thanks - could you try this branch? https://github.com/rlbdv/lokke/tree/tmp-binary-executables If that works, then I might like to merge it and we can rebase the rest of your improvements on top.

rlbdv commented 3 years ago

Oh, and given the autotools-related changes you might or might not need to either work from a clean tree, or run some or all of the initial setup commands, i.e. if all else fails:

autoreconf -fi
./configure
make clean
make  # or make -j5 or whatever
aconchillo commented 3 years ago

Thanks - could you try this branch? https://github.com/rlbdv/lokke/tree/tmp-binary-executables If that works, then I might like to merge it and we can rebase the rest of your improvements on top.

Much better, but there are still issues with the clojure related tests that use ./lokke -0 instead of ./guile. The problem is the same I found yesterday, all arguments in the first line are treated as a list of single arguments. Added a (pk args) to main.scm and it shows:

;;; (("./lokke" "-0" ";;" "-*-clojure-*-" "./test/clojure-basics"))

So I believe it makes sense to do this instead:

#!./lokke -0
!# ;; -*-clojure-*-

which is the same approach you already have for the other tests:

#!./guile
!# ;; -*-scheme-*-

So, certainly using an executable is a better solution than a script since that works out of the box in macOS already without having to add /usr/bin/env -S.

For scripts like example/nbody/nbody we have to do this instead:

#!./lokke -0
;; -*-clojure-*-
(apply run "-l" %0 "-a" "nbody/-main" "--" %&)
!#

That is add the mode line to the second line since Emacs still looks there.

rlbdv commented 3 years ago

Hmm, so I just tested on FreeBSD 12.2, which I'd expect to have similar behaviors on this front, and test/clojure-collection worked fine, as did gmake check on the branch. I wondered if there might be a chance you had lingering changes with respect to tmp-binary-executables. (If you're not sure, suppose you could try a fresh clone or reset of that branch.)

On the other hand, maybe it is something macos specific.

On the positive side, with some minor changes, things seem to work fine on FreeBSD now. I'll queue those up for my next update.

aconchillo commented 3 years ago

Hmm, so I just tested on FreeBSD 12.2, which I'd expect to have similar behaviors on this front, and test/clojure-collection worked fine, as did gmake check on the branch. I wondered if there might be a chance you had lingering changes with respect to tmp-binary-executables. (If you're not sure, suppose you could try a fresh clone or reset of that branch.)

On the other hand, maybe it is something macos specific.

On the positive side, with some minor changes, things seem to work fine on FreeBSD now. I'll queue those up for my next update.

I cloned your repo fresh and same problem:

❯ cat test/clojure-comparison.log
Backtrace:
In ice-9/boot-9.scm:
  1736:10  5 (with-exception-handler _ _ #:unwind? _ # _)
In unknown file:
           4 (apply-smob/0 #<thunk 10a23b5a0>)
In lokke/main.scm:
   304:38  3 (lokke-main _)
   262:25  2 (run-script _ _)
In ice-9/ports.scm:
   440:11  1 (call-with-input-file ";;" #<procedure 10adb5e40 at lo?> ?)
In unknown file:
           0 (open-file ";;" "r" #:encoding #f #:guess-encoding #f)

ERROR: In procedure open-file:
In procedure open-file: No such file or directory: ";;"
aconchillo commented 3 years ago

On the other hand, maybe it is something macos specific.

I think that might be the case. macOS splits all arguments and that might be different than FreeBSD.

I still think that keeping the mode line out of the first line would be a good idea, for the same reason you had to do:

#!./guile
!# ;; -*-scheme-*-
aconchillo commented 3 years ago

One last thing, Emacs also says you should not use first line

In shell scripts, the first line is used to identify the script interpreter, so you cannot
put any local variables there. To accommodate this, Emacs looks for local variable
specifications in the second line if the first line specifies an interpreter.

From https://www.gnu.org/software/emacs/manual/html_node/emacs/Specifying-File-Variables.html

rlbdv commented 3 years ago

I just updated main with I think should include full support for FreeBSD, and rebased tmp-binary-executables on that. Though I suspect it might not improve the macos issue yet. And while I'm not opposed to moving the mode lines if we need to, I'd first like to make sure I understand the underlying issue because while it's true in general that you shouldn't put it there, I believe what's actually allowed should depend on the #! interpreter, and we're now the (sole) interpreter.

rlbdv commented 3 years ago

Oh, and originally (when working on (lokke main) a good while back) I'd meant for this to work (specifically wrt -0, so now that I have a minute, I'll go remember exactly what I'd intended in there, and I might have just made a mistake.

rlbdv commented 3 years ago

OK, had time to investigate, and agree that best solution is the one you've suggested, i.e. forbid arguments other than -0 on the #! line. I went ahead and updated just that bit and lokke.1, and added some checking in main. I also pushed the "binary executable" work, so if you'd like to rebase on top of main, we can pursue the tests.

Thanks again for the help.

aconchillo commented 3 years ago

OK, had time to investigate, and agree that best solution is the one you've suggested, i.e. forbid arguments other than -0 on the #! line. I went ahead and updated just that bit and lokke.1, and added some checking in main. I also pushed the "binary executable" work, so if you'd like to rebase on top of main, we can pursue the tests.

Thanks again for the help.

Thank you! I think it all works now (actually I'm now having issues compiling with Guile 3.0.5 because of some deprecations).

So my PR ended up in just basically README.md updates, github actions and other minor things. I commented out is_latin1_plusminus since it's unused.

rlbdv commented 3 years ago

Aleix Conchillo Flaqué notifications@github.com writes:

So my PR ended up in just basically README.md updates and other minus things. I commented out is_latin1_plusminus since it's unused.

Oh, right, I think I've removed that in main?

aconchillo commented 3 years ago

Aleix Conchillo Flaqué notifications@github.com writes: So my PR ended up in just basically README.md updates and other minus things. I commented out is_latin1_plusminus since it's unused. Oh, right, I think I've removed that in main?

You are right, that was a merge conflict I solved wrong. Removed and PR updated.

rlbdv commented 3 years ago

OK, I think I've incorporated all of this into main (and then reworked the broader workflow a bit). I also removed the CFLAGS and and CFLAGS settings since I think we don't actually need them (I think configure determines those via pkgconf). In any case, thanks again for the help.

aconchillo commented 3 years ago

Cool, thank you! @rlbdv let me know if you are interested in the README update or the github actions for macos which were available in this PR. Happy to help.

rlbdv commented 3 years ago

Aleix Conchillo Flaqué notifications@github.com writes:

Cool, thank you! @rlbdv let me know if you are interested in the README update or the github actions for macos which were available in this PR. Happy to help.

Oh, I think I included them? I just rearranged things a bit after incorporating your work. i.e. macos is included in the main.yaml workflow, and your code was folded in to that and the new ci/bin/prep-and-test, etc. Your documentation changes ended up mostly in the new INSTALL file. There should be some additional information in the relevant commit messages.

But please let me know if what we ended up with doesn't seem suitable.

Thanks again

aconchillo commented 3 years ago

Duh, I see it now. All good!