tagua-vm / parser

Safe, fast and memory efficient PHP parser (lexical and syntactic analysers, and the Abstract Syntax Tree)
http://tagua.io/
119 stars 15 forks source link

fix Dockerfile #97

Closed mathroc closed 7 years ago

mathroc commented 7 years ago

also upgrades rust, add docker-compose.yml & removes unused llvm symlink (llvm is not installed in this image)

Hywan commented 7 years ago

Thank you for your PR :-). However, I was wondering if keeping Docker for this library makes sense or not… Thoughts?

mathroc commented 7 years ago

@Hywan you mean this library: the parser or tagua-vm/tagua-vm too ?

the use case for me is that I don't install rust (or php, node, etc) on my OS but only use docker when working on a project.

this means when trying to work on this (because hopefully I'll make some time for it) I'll be able to do docker-compose run --rm dev cargo test (run dev cargo test with alias) for testing my changes on the parser

but I don't mind if the Dockerfile & docker-compose.yml are not included in the project, I'll just have them outside git (and probably in a branch of my fork in case I lose them)

Hywan commented 7 years ago

@mathroc I mean this library only. If this is useful for development, then why not keeping it. I am not against. But I really don't like Docker and I don't want to maintain it (I am not able to maintain it neither). So you can be in charge of it, or maybe @jubianchi :-)?

Hywan commented 7 years ago

Would it be difficult to move Docker stuff into a Build/ root directory? We will move Travis over there too.

mathroc commented 7 years ago

Hi, I didn't had time to work on tagua-vm and likely won't have in the near future. also I've switch to rustup to manage rust installs

so I guess unless someone is interested in updating the Dockerfile it might be removed as it's currently not working

Pierozi commented 7 years ago

Hello there, @mathroc your PR work well with few change. Bump version of rust to 1.17 and add command: tail -f /dev/null to keep container running in docker-compose

Hywan commented 7 years ago

I still wonder if it makes sense to have a Docker file for a library like tagua-parser :-/. Thoughts?

mathroc commented 7 years ago

@Pierozi I tried to upgrade to 1.17 but cargo test still fails, its working on your end ? also, I don't understand why you want to add command: tail -f /dev/null, what use is it to start the container doing nothing ?

@Hywan I don't know either, maybe someone could set up a repository with everything needed to work on tagua (both parser & vm) on docker ? and the README here could link to that repo instead ?

Hywan commented 7 years ago

@mathroc It makes sense to have a Docker file on tagua-vm, but not a tagua-parser :-/. Unless someone agrees to maintain it, I would like to remove it.

mathroc commented 7 years ago

yes a Dockerfile on tagua-vm makes sense, especially if it build a production ready tagua-vm that can be pushed on docker hub

Pierozi commented 7 years ago

I think docker still make sense, in my case the tagua test failed due to the outdated version of Rust in my laptop. I've open PR with my test regarding Cross if you are interested in.

mathroc commented 7 years ago

replaced with #106

mathroc commented 7 years ago

thx @Pierozi