Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge eval and evalAST in purescript implementation #631

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

asarhaddon
Copy link
Contributor

Hello.
These changes should be part of #592 , but

  • the merge request adding the purescript implementation has been accepted before it,
  • I had to update some settings for the latest purescript. @mrsekut, could you please review them ?.

Copy link
Contributor

@mrsekut mrsekut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!!!

Copy link
Owner

@kanaka kanaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the Dockerfile needs to be updated. Probably something like this:

FROM ubuntu:24.04

##########################################################
# General requirements for testing or common across many
# implementations
##########################################################

RUN apt-get -y update

# Required for running tests
RUN apt-get -y install make python3
RUN ln -fs /usr/bin/python3 /usr/local/bin/python

RUN mkdir -p /mal
WORKDIR /mal

##########################################################
# Specific implementation requirements
##########################################################

RUN DEBIAN_FRONTEND=noninteractive apt-get -y install g++ libreadline-dev nodej
s npm

# Install purescript and deps
RUN apt-get install -y git libtinfo6
RUN npm install -g --unsafe-perm purescript spago

ENV NPM_CONFIG_CACHE /mal/.npm
ENV HOME /mal

@asarhaddon
Copy link
Contributor Author

Now the build system complains that esbuild is not installed.

@kanaka
Copy link
Owner

kanaka commented Aug 8, 2024

Looks like that error can be resolved like this:

--- a/impls/purs/Makefile
+++ b/impls/purs/Makefile
@@ -9,7 +9,7 @@ OTHER_SRCS = src/Readline.js src/Readline.purs src/Types.purs src/Reader.purs \
 all: $(BINS)

 $(BINS): %.js: src/%.purs $(OTHER_SRCS) node_modules/readline-sync
-       spago bundle-app --main $($(<:src/%=%)) --to $@
+       spago bundle-app --platform=node --main $($(<:src/%=%)) --to $@


 node_modules/readline-sync:

@kanaka
Copy link
Owner

kanaka commented Aug 9, 2024

@asarhaddon I have no idea what's causing the most recent failure. I poked around a bit but didn't see anything obvious.

@kanaka kanaka merged commit c5788b3 into kanaka:master Aug 26, 2024
4 checks passed
@asarhaddon
Copy link
Contributor Author

The same error appears without the merge-eval, without readline and with argv, readString, writeString from the node-process module instead of a local FFI to javascript. I am giving up the update part.

@asarhaddon asarhaddon deleted the merge-eval-purs branch August 26, 2024 18:02
@kanaka
Copy link
Owner

kanaka commented Aug 26, 2024

@asarhaddon works for me. There are a lot of implementations that could/should be updated from very old version of the docker image/compiler/interpreter. I'm fine with not doing that as part of these updates if it's proves problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants