jvail / glpk.js

GLPK for browser & node
GNU General Public License v3.0
106 stars 19 forks source link

Set up build process using docker #12

Closed rregue closed 4 years ago

rregue commented 4 years ago

This is an attempt to simplify building the project using docker and the official docker image for emscripten/emsdk.

jvail commented 4 years ago

Thank you @rregue - good idea. I have added a tiny comment in the Dockerfile.

rregue commented 4 years ago

@jvail thanks for the review! I just made the change. Do you want me to also push the files generated when building with docker? Something else I noticed is that the docker image emscripten/emsdk:1.39.18 uses node 12.18 but in this repository we are using node 11 due to the node-glpk dev dependency. I am not sure if this can create any sort of problems going forward. I did run the tests suite with both node 11 and node 12 (commenting out node-glpk code) and I do get the same results for the test problems.

jvail commented 4 years ago

Thank you @rregue.

@jvail thanks for the review! I just made the change. Do you want me to also push the files generated when building with docker?

Not sure I understand what you mean with "push". I would not update the dist files in master right now since I don't expect any change.

Something else I noticed is that the docker image emscripten/emsdk:1.39.18 uses node 12.18 but in this repository we are using node 11 due to the node-glpk dev dependency. I am not sure if this can create any sort of problems going forward. I did run the tests suite with both node 11 and node 12 (commenting out node-glpk code) and I do get the same results for the test problems.

I think it would be better to keep our own node version instead of relying on emsdk's version since node glpk is not updated frequently - it seems. Running the tests (which are not super extensive - I admit) inside docker would require to run the npm install as well and the test could be run using the local node package. However, then I think it would be better not to mount the entire directory and instead just mound a dist folder (which does not exists currently :\ ). Yes, some work to do.

Unfortunately GLPK (GNU) seems to be unmaintained currently and I am a bit unsure if it makes sense to invest a lot of time here. Without someone maintaining the C GLPK code it is hard to see a long future for this package here. But I will keep maintaining it since I did not come across a good alternative for LP & JS.

rregue commented 4 years ago

Thanks for getting this merged.

Not sure I understand what you mean with "push". I would not update the dist files in master right now since I don't expect any change.

Yes, so that is what I meant, updating the dist files, sorry for not being clear. I do see the files changing this is why I raise it.

Unfortunately GLPK (GNU) seems to be unmaintained currently and I am a bit unsure if it makes sense to invest a lot of time here. Without someone maintaining the C GLPK code it is hard to see a long future for this package here. But I will keep maintaining it since I did not come across a good alternative for LP & JS.

I came to the same realization, there isn't a good alternative for LP/MIP in JS. For the use case I have in mind, I would love to be able to add the following features:

If you think this is worthwhile, I can create issues to track them and eventually make the relevant changes.

jvail commented 4 years ago

Hi @rregue,

yes, of course. Please go ahead. I have currently no project were I use the library and therefore not many ideas how to improve it. I am happy to help if you have ideas to contribute code. I have created two issues were I started collecting ideas for improvements and a new major release with api changes: e.g. passing an optional "options" obj to the solve function. There are certainly lots of features in GLPK that are not exposed currently. But I would like to keep an eye on the size of the wasm file. Every feature made available might increase the size. Maybe we could define a minimal set and then add compiler options to customize the build and enable / disable features.

rregue commented 4 years ago

Awesome, this is great. Good call out on keeping an eye on the size of the wasm file. Also, I could not see the change you mention in here: a new major release with api changes: e.g. passing an optional "options" obj to the solve function. This would be great, as we are currently limited to only pass the message level.