search

MycroftAI / mimic-recording-studio

Mimic Recording Studio is a Docker-based application you can install to record voice samples, which can then be trained into a TTS voice with Mimic2
Apache License 2.0
496 stars 114 forks source link

Remove yarn installation from frontend Dockerfile #24

Closed Willem3141 closed 4 years ago

Willem3141 commented 4 years ago

It seems that the node:latest image already includes yarn, which causes an error message when trying to build the container:

> sudo docker build .
Sending build context to Docker daemon  982.5kB
Step 1/4 : FROM node:latest
 ---> 2f1ff44a8bb5
Step 2/4 : RUN npm install -g yarn
 ---> Running in 5183549b5eb0
npm ERR! code EEXIST
npm ERR! syscall symlink
npm ERR! path ../lib/node_modules/yarn/bin/yarn.js
npm ERR! dest /usr/local/bin/yarn
npm ERR! errno -17
npm ERR! EEXIST: file already exists, symlink '../lib/node_modules/yarn/bin/yarn.js' -> '/usr/local/bin/yarn'
npm ERR! File exists: /usr/local/bin/yarn
npm ERR! Remove the existing file and try again, or run npm
npm ERR! with --force to overwrite files recklessly.

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2020-02-04T22_12_42_031Z-debug.log
The command '/bin/sh -c npm install -g yarn' returned a non-zero code: 239

This causes the build to fail. Removing the yarn installation entirely fixes this issue.

forslund commented 4 years ago

Hi, I just tested this and it works nicely with your fix. Thank you for providing it.

Before merging we require you to sign the Contributor License Agreement to ensure that you retain the rights to your submission while granting Mycroft right to use it.

Willem3141 commented 4 years ago

@forslund Signing a CLA seems overkill for such a trivial change. I hereby release my contribution under CC0 (public domain). If that is not sufficient, I don't mind if you close this MR, and remove the line installing yarn independently :slightly_smiling_face:

forslund commented 4 years ago

I understand @Willem3141, it's sort of a line drawing thing, if we allow this then we could allow this little bigger change...so we require it on everything. I'll talk it over with the others and if not OK I'll recreate your PR as you suggested.

In any case huge thanks for flagging and providing a solution.

forslund commented 4 years ago

Thanks for this @Willem3141, We talked it over and I ended up recreating the PR (#25). Thanks for providing the solution.