tezos-checker / checker

An in-development "robocoin" system for the Tezos blockchain
24 stars 16 forks source link

Add a docker-based workflow #234

Closed utdemir closed 3 years ago

utdemir commented 3 years ago

This PR introduces a Docker-based workflow for developing checker.

It provides instructions to build a Docker image which contains Nix alongside with a prepopulated /nix/store with Checker's dependencies. It expects a Checker repository to be mounted under /nix, and automatically enters to the nix-shell on entrypoint.

We had a chat with @gkaracha and decided to remove VSCode from the mix and just provide a simple Docker image. They can still edit the code using vscode, and see the errors by running make under the docker container (Or they can do fd | entr -c make build-ocaml which is exactly the same way that I develop checker, so in my opinion it is not horrible :) ).

I considered a GitHub action to build and test this Docker image, but then I decided against it because I don't think this will be changed frequently.

utdemir commented 3 years ago

One concern I have (but that's more generally a docker issue I guess) is that I couldn't edit the files inside the image since no editor was available.

My intention is that you'd edit the files outside of the container, and since the host directory is mounted inside the container the changes will be seen inside the container too. Maybe we should clarify it on the README.

gkaracha commented 3 years ago

My intention is that you'd edit the files outside of the container, and since the host directory is mounted inside the container the changes will be seen inside the container too. Maybe we should clarify it on the README.

Ah, right, I didn't think of that :facepalm: Yes, I can confirm that this approach works. Perhaps add that line to the README in case others are as absent-minded as I am :sweat_smile:

gkaracha commented 3 years ago

@utdemir I thought I'd try out the end-to-end tests too

$ CHECKER_DIR=$(nix-build -A michelson --arg e2eTestsHack true --no-out-link) python e2e/main.py

but they seem to fail. You can see the whole output here: https://gist.github.com/gkaracha/fc49146d3ead446240c6907cdf26a354

utdemir commented 3 years ago

Oh, right. That likely is because the Docker socket the clients use to connect to the system docker daemon is not available inside the container. Could you try with passing

-v /var/run/docker.sock:/var/run/docker.sock

to docker run?

gkaracha commented 3 years ago

Oh, right. That likely is because the Docker socket the clients use to connect to the system docker daemon is not available inside the container. Could you try with passing

-v /var/run/docker.sock:/var/run/docker.sock

to docker run?

You mean like this?

$ docker run -it -v /var/run/docker.sock:/var/run/docker.sock -v "$PWD:/mnt" checker-devcontainer

Gives me the same error :thinking:

github-actions[bot] commented 3 years ago
Gas costs d07694a071e5877739aa36641294b3a6078f3da4 031f04d7245d969be35bb2ce18475bf94aa1b2d6 Diff
touch 538767 None -538767
remove_liquidity 73903 None -73903
add_liquidity 73651 None -73651
buy_kit 69637 None -69637
sell_kit 68764 None -68764
deactivate_burrow 59288 None -59288
withdraw_collateral 58284 None -58284
set_burrow_delegate 54691 None -54691
activate_burrow 54542 None -54542
deposit_collateral 53735 None -53735
mint_kit 51869 None -51869
burn_kit 51613 None -51613
create_burrow 48918 None -48918
touch_burrow 44409 None -44409
update_operators 41672 None -41672
transfer 39928 None -39928
Entrypoint sizes d07694a071e5877739aa36641294b3a6078f3da4 031f04d7245d969be35bb2ce18475bf94aa1b2d6 Diff
touch 56614 None -56614
mark_for_liquidation 17093 None -17093
touch_liquidation_slices 14508 None -14508
cancel_liquidation_slice 12177 None -12177
liquidation_auction_place_bid 2229 None -2229
remove_liquidity 2038 None -2038
add_liquidity 1967 None -1967
deactivate_burrow 1603 None -1603
mint_kit 1588 None -1588
burn_kit 1561 None -1561
withdraw_collateral 1488 None -1488
buy_kit 1376 None -1376
sell_kit 1372 None -1372
activate_burrow 1292 None -1292
deposit_collateral 1163 None -1163
create_burrow 1097 None -1097
liquidation_auction_claim_win 1062 None -1062
set_burrow_delegate 1059 None -1059
touch_burrow 660 None -660
update_operators 468 None -468
receive_price 155 None -155
receive_slice_from_burrow 119 None -119
Test coverage d07694a071e5877739aa36641294b3a6078f3da4 031f04d7245d969be35bb2ce18475bf94aa1b2d6 Diff
ctez.ml 100 None -100
parameters.ml 100 None -100
lqt.ml 100 None -100
cfmmTypes.ml 100 None -100
checkerEntrypoints.ml 100 None -100
ptr.ml 100 None -100
liquidationAuctionTypes.ml 100 None -100
fixedPoint.ml 100 None -100
common.ml 100 None -100
error.ml 100 None -100
mem.ml 100 None -100
checkerTypes.ml 100 None -100
fa2Interface.ml 100 None -100
kit.ml 100 None -100
constants.ml 100 None -100
cfmm.ml 100 None -100
tok.ml 97.37 None -97.37
burrow.ml 96.26 None -96.26
sliceList.ml 95.38 None -95.38
checkerMain.ml 92.86 None -92.86
avl.ml 91.05 None -91.05
checker.ml 90.08 None -90.08
liquidationAuction.ml 77.83 None -77.83
TOTAL 93.96 None -93.96
utdemir commented 3 years ago

Gives me the same error

It isn't quite the same error :). The previous one was:

urllib3.exceptions.ProtocolError: ('Connection aborted.', FileNotFoundError(2, 'No such file or directory'))

But the new one is:

urllib3.exceptions.ProtocolError: ('Connection aborted.', PermissionError(13, 'Permission denied'))

The previous error was about that socket not existing on the docker container. Turns out the new one is a permission error, where /var/run/docker.sock is only has rw permissions for either the root user or the docker group. SInce we're not root, and the docker group does not exist on the container, we can not use the socket. I guess one solution would be to add a docker group to the container and ensure that it has the same gid of the docker group on the host machine.

However, I do think we can do it later. We're only missing e2e tests, and they are not very useful for development since they takes ages. One can always push a commit and let the CI do it when absolutely necessary.

gkaracha commented 3 years ago

It isn't quite the same error :).

Ah, sorry, you're absolutely right. I skimmed the output too fast :facepalm:

However, I do think we can do it later. We're only missing e2e tests, and they are not very useful for development since they takes ages. One can always push a commit and let the CI do it when absolutely necessary.

Yeah, doing it later sounds good to me, it's certainly not a priority. I would change the README a little though, to be clear that we are aware of this limitation. Something like this perhaps:

- Within the image, you can use the `make` based workflow above.
+ Within the image, you can use the `make` based workflow above (but not run the e2e tests, currently).
dorranh commented 3 years ago

@utdemir, I'm going to go ahead and close this since I think it was superseded by #245. Please ping me and re-open the PR if there are changes which aren't covered by #245 though. :slightly_smiling_face: