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

Remove eslc indirection, update LiveScript dependency #40

Closed
wants to merge 2 commits into from

Conversation

dead-claudia
Copy link
Contributor

@anko

It's unlikely at this stage that any consumers are depending on the location
of the binary, and it's not even documented within this repo at all, but
hardcoded throughout the docs tests. The executable indirection is removed
to now point to the corresponding file in lib. All the former references to
the old binary now run node lib/cli.js.

This will also open up the possibility of making this buildable on Windows
platforms, as it's now almost trivial to drop make as a required dependency
instead aliasing everything to npm run, as lsc works with directories as
well as files, and there's rimraf (already a dev dependency) to replace
make clean, and lsc creates nonexistent directories before compiling from
a source directory.

Also, the newest minor update of LiveScript is purely additive and bug fixes
except for generators no longer being automatically hushed, which shouldn't
affect this repo.

http://livescript.net/#changelog

impinball added 2 commits December 26, 2015 01:29
It's unlikely at this stage that any consumers are depending on the location
of the binary, and it's not even documented within this repo at all, but
hardcoded throughout the docs tests. The executable indirection is removed
to now point to the corresponding file in `lib`. All the former references to
the old binary now run `node lib/cli.js`.

This will also open up the possibility of making this buildable on Windows
platforms.

Also, the newest minor update of LiveScript is purely additive and bug fixes
except for generators no longer being automatically hushed, which shouldn't
affect this repo.

http://livescript.net/#changelog
@dead-claudia
Copy link
Contributor Author

Yeah...I had to add that dependency to get both Travis and my local tests to stop screaming at me. Don't know why that wasn't already added. 😦

@anko
Copy link
Owner

anko commented Jan 1, 2016

The eslc binary fails on Linux with this: without a shebang line to specify it should be run with node, the kernel tries to run it as a shell file.

We could fix it by adding a conditional to the build, prepending that line to lib/cli.js

diff --git a/makefile b/makefile
index 353da54..35f55b7 100644
--- a/makefile
+++ b/makefile
@@ -9,7 +9,10 @@ all: $(LIB)

 lib/%.js: src/%.ls
    @mkdir -p lib/
-   lsc --output lib --bare --compile "$<"
+   @if [ "$@" = "lib/cli.js" ] ; then \
+       echo "#!/usr/bin/env node" > $@ ;\
+   fi
+   lsc --output lib --bare --compile --print "$<" >> $@

 clean:
    @rm -rf lib/

—but I'd like to minimise logic in the build process. Having bin/eslc clearly denote the executable entry-point is clearer, and easier to maintain e.g. if eslisp needs more executables later.

What do you think?


The missing uuid dependency is my fault! I had accidentally removed it, and because I have Travis set up to cache node_modules/ since 8d57061, it didn't catch the mistake right away either.

I think it's best to disable that cache altogether actually. I've had to manually delete Travis' cache a few times for similar issues too.


Getting eslisp to build on Windows would be neat, but I have no Windows machine to test it on, or a good understanding of what's necessary. If you do, could you create an issue about it?

@anko
Copy link
Owner

anko commented Jan 1, 2016

For reference, I've cherry-picked the b110323 uuid dependency fix to master (as a56105f), and updated LiveScript in 78293f4.

@dead-claudia
Copy link
Contributor Author

Good point. On that note, I'll just make a new PR that just cleans up the eslc binary.

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.

None yet

2 participants