online-bridge-hackathon / DDS

An api that returns DDS results for a given deal or partial deal
https://bit.ly/bridge-hackathon
Apache License 2.0
4 stars 6 forks source link

Added a 'start_local_server' make target. #76

Closed tameware closed 4 years ago

tameware commented 4 years ago

Also updated the docs to use the new target and cleaned them up a bit.

tameware commented 4 years ago

I'll add a "logs" directory and a corresponding .gitignore entry. I would not think to look for logs in .build – it has a different purpose.

On Mon, Jul 13, 2020 at 11:09 AM suokko notifications@github.com wrote:

@suokko commented on this pull request.

In Makefile https://github.com/online-bridge-hackathon/DDS/pull/76#discussion_r453800286 :

@@ -78,3 +78,7 @@ curl_local curl_prod: "N":["C7", "H6", "H7", "H9", "CJ", "SA", "S8", "SQ", "D5", "S5", "HK", "C8", "HA"], \ "E":["H2", "H5", "CQ", "D9", "H4", "ST", "HQ", "SJ", "HJ", "DQ", "H3", "C9", "CK"] }}' \ ${CURL_URL} + +start_local_server: libdds-build

  • pip3 install -q -r requirements.txt
  • python3 -m src.api

Yes. Typo. There is other typos like missing second dollar for the subshell syntax.

Yeah. Output probably should redirected. Maybe inside the .build directory.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/online-bridge-hackathon/DDS/pull/76#discussion_r453800286, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABC4PYAU2BEW3EJ43EM7OLLR3M5UNANCNFSM4OYECZSA .

suokko commented 4 years ago

I'll add a "logs" directory and a corresponding .gitignore entry. I would not think to look for logs in .build – it has a different purpose.

logs is a better idea. Maybe also and echo line advertising where the server log is written.

I just remembered the background starting has to wait for service to open the port for listening if local_curl target depends directly on server startup. My own similar startup had advantage that opening a web page using xdg-open was much slower than node.js http-server startup. But in our case curl is quite likely to be faster than python. Netcat might help here but I don't know if it is the best (Maybe curl can do the waiting too so one less dependency) https://unix.stackexchange.com/questions/5277/how-do-i-tell-a-script-to-wait-for-a-process-to-start-accepting-requests-on-a-po

tameware commented 4 years ago

If I run make start_local_server and then (almost) immediately run make curl_local the curl succeeds.

If I run them both on the same command line, e.g., make start_local_server && make curl_local the curl fails. I don't see that as a problem. The curl is idempotent - it can just be run again.

tameware commented 4 years ago

Any reason to use ! || instead of &&? With the latter the command would be:

[ -e ${pidfile} ] && (kill $$(cat ${pidfile}) && rm ${pidfile})
suokko commented 4 years ago

Any reason to use ! || instead of &&? With the latter the command would be:

[ -e ${pidfile} ] && (kill $$(cat ${pidfile}) && rm ${pidfile})

If pidfile doesn't exists, then test returns false and whole line fails. The easiest method to fix that was to invert the logic. That would have deserved a comment. The success without pidfile is important because I wanted to add dependency from the server start to the stop to make sure previous server is stopped before a new one.

Then kill can return failure if local service has been stopped in which case pidfile should be removed even if kill fails. That why the second and should be a semicolon.

tameware commented 4 years ago

Second && replaced by ;

My other question still stands. Isn't [ condition ] && the same as [ ! condition ] || ?

suokko commented 4 years ago

Second && replaced by ;

My other question still stands. Isn't [ condition ] && the same as [ ! condition ] || ?

No. It is equal to [ condition ] && <command> || true in some cases. The last true is the important difference. There is no completely equal condition without not operator.

tameware commented 4 years ago

I must be missing something. What's the failing case?

$ [ x = x ] && echo abc
abc
$ [ x = y ] && echo abc
$ [ ! x = x ] || echo abc
abc
$ [ ! x = y ] || echo abc
$
suokko commented 4 years ago

I must be missing something. What's the failing case?

$ [ ! x = y ] || echo abc; echo $?
0
$ [ x = y ] && echo abc; echo $?
1
tameware commented 4 years ago

Yes, the exit status is that of the first command since the second is always true. But how does that affect things in the case in the Makefile? We want to continue execution if and only if the test passes - we're just taking advantage of Boolean short-circuiting.

This is not a big deal here - I'm pretty suite either version would work - I just want to understand your concern.

suokko commented 4 years ago

If return value isn't zero then makefile will generate error. The expression return zero when there is no pidfile which requires using negated existence check.

tameware commented 4 years ago

Got it!

This is the command output from make start_local_server:

git submodule update --init --rebase
make -C libdds/.build
[100%] Built target dds
[ ! -e logs/server.PID ] || (kill $(cat logs/server.PID) ; rm logs/server.PID)
python3 -m src.api > logs/src.api.2020-07-16T19:57:09.log 2>&1 & echo $! > logs/server.PID

Is that enough of a clue or should I add an echo as well, per your suggestion?

suokko commented 4 years ago

I tough I replied earlier. But I guess it was only a simulated reply in my head.

The command line message should make it clear where logs can be found..

tameware commented 4 years ago

Ready for another look!

tameware commented 4 years ago

Ready for next pass!

suokko commented 4 years ago

Looks good to me. I don't know how to change the review status from the mobile version. I will be home in a moment.