molovol / MoloVol

MoloVol is a free, cross-plattform, scientific software for volume and surface computations of single molecules and crystallographic unit cells.
https://molovol.com
MIT License
22 stars 4 forks source link

Install MoloVol directly to server #141

Closed jmaglic closed 7 months ago

jmaglic commented 7 months ago

Instead of building the executable in a build directory and running it directly from there, it is much safer to build a deb file for installation and use it to install MoloVol to the system. This makes MoloVol accessible directly through the command "molovol" by adding the executable to the PATH. Hence launch_headless.sh can directly call the command instead of running the executable from the build folder.

The instructions for installation have been added to the Dockerfile.

The benefit of this is that resource files are installed on a system directory instead of in a local directory. This makes the setup less susceptible to subtle changes, such as changing the working directory, binary directory, or resource directory.

This change required an adjustment in app.py, so that the system elements file is accessed instead of the local elements file.

BSVogler commented 7 months ago

I would argue that relying on local files makes it easier to maintain and build than using system paths. This adds an additional step that makes it more dependent on the os. You can also make the docker file modify the PATH variable and call "molovol" directly without relying on the package manager. Ubuntu is only the base image since that is required for wxwidget. I tried also to use alpine before. Long-term it would make sense to allow building the headless without wxwidget and then the image could be a lot smaller without the os.

jmaglic commented 7 months ago

I see your point. I would suggest, that we switch to a system installation because it will make the server more robust. Until we change the OS the server should not break, even if we change the build folder or the working directory. This would prevent issues like the latest crash.

When we implement a truly headless version (which I am somewhat hesitant to do right now because the current version works and I want to work on more impactful changes) we will likely have to rework some of the installation scripts and then we can go back to local paths.

BSVogler commented 7 months ago

On the server I run it via docker. So unless I start managing and deploying deb files it would not make it more robust.

This would prevent issues like the latest crash.

Hence, I don't follow this point. What happened was that the build was changed and the dockerfile became incompatible.

This MR has two changes, one is switching to deb file and the other is the fix for the missing files. I wanted to argue in favor or releasing the latter (asap, as it is broken as of now) while excluding the deb file. However, as I must admit, this is not a strong argument on my side so I welcome releasing this whole MR.

jmaglic commented 7 months ago

Then I will merge the entire PR. I only now realised that I may not have told you, but what motivated the other change was that even after I created the missing directories manually the app was still broken because it could not find the input files. I don't know if switching to CMake changed the working directory or something but there was some issue that would have been avoided if the app had been installed globally.

We may reverse this change and go back to local files. I admit that my experience with Docker is very limited and I don't have a feeling for best practices and for what is or isn't easy to maintain.

In any case, I hope that this resolves all current issues. Please let me know if you redeploy the app, so that I can run a few tests.