Update the build command for OLC #122
Conversation
csilvers
left a comment
csilvers
left a comment
There was a problem hiding this comment.
I like that; OLC should be responsible for its own installatio
setup.sh
Outdated
| install_our_lovely_cli() { | ||
| cd "$DEVTOOLS_DIR/our-lovely-cli" | ||
| npm install | ||
| make |
There was a problem hiding this comment.
On second thought, we should have an explicit make target here, rather than just depending on the first one being the one to install. I'm not even sure what it should be, looking at the (confusing) Makefile. make yarn? make all? You may want to ask in #github-prs.
There was a problem hiding this comment.
@csilvers we're having problems with our Makefile, and it's been on my list to reach out to you to chat w you (when you have time), as everyone says you're the "make expert" here. I wonder if we should solve that first, since the solution could be to just get rid of the Makefile altogether...
There was a problem hiding this comment.
@lillialexis am happy to chat with you! My calendar is accurate if you want to find time tomorrow.
There was a problem hiding this comment.
oh, wait, maybe make all doesn't do yarn build, but I'll fix that in OLC momentarily...
There was a problem hiding this comment.
Alright, I updated it to make all assuming that will be the final command.
| cd "$DEVTOOLS_DIR/our-lovely-cli" | ||
| npm install | ||
| make all |
There was a problem hiding this comment.
Now that you're using make, a better alternative is to just do:
make -C "$DEVTOOLS_DIR/our-lovely-cli" all
Then we don't have to do the cd, which is always nice to avoid in shell scripts.
lillialexis
left a comment
lillialexis
left a comment
There was a problem hiding this comment.
Ok, hold up, because now John is pushing back on the make all in the OLC Makefile... so sorry that this has been such a headache to coordinate! https://github.com/Khan/our-lovely-cli/pull/743
lillialexis
left a comment
lillialexis
left a comment
There was a problem hiding this comment.
Ok, I just merged my new command in with make all
Summary
The build command in
install_our_lovely_cli()is not quite right because:yarnso doingnpm installcauses a warning about apackage-lock.jsoncreated by a different package manager when doing subsequent runs ofyarnThis change updates it to run
make all.Test plan