Dockerfile fixed. #308

aahutsal · 2019-07-02T10:20:18Z

docker/build.sh works now.

parity-cla-bot · 2019-07-02T10:20:20Z

It looks like @agutsal hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

aahutsal · 2019-07-02T10:31:34Z

[clabot:check]

parity-cla-bot · 2019-07-02T10:31:36Z

It looks like @agutsal signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

aahutsal · 2019-07-02T10:32:27Z

@chevdor seems I've fixed Dockerfile. It's moved to docker directory, as build.sh expect it to live there. Please review and merge.

chevdor · 2019-07-03T11:24:57Z

I don´t think this is a good idea to move the Dockerfile into a sub folder (although it would be cleaner).

The idea is nice but Docker does not accept reference outside of its path. So having the Dockerfile at the root allows reaching any part of the project.

The changes you made to the file itself look good to me and adding clang is long overdue, thanks for bringing it as PR!
Can you bring the Dockerfile back up?

aahutsal · 2019-07-03T14:11:34Z

This will require to change docker/build.sh and probably something else. Currently it works if running from project root via docker/build.sh. To me it also looks cleaner. If we mention that command docker/build.sh in README - it might work. What do you think?

ddorgan · 2019-07-03T16:47:51Z

@TriplEight is there a reason to keep this? Since we're building docker images on each master merge.

TriplEight · 2019-07-03T20:45:47Z

@ddorgan this is deprecated.
In CI we copy the Dockerfile from scripts/docker/ to the dir with the compiled binary and then run docker build --tag parity/polkadot:$VERSION . there.

aahutsal · 2019-07-04T06:42:04Z

@TriplEight @ddorgan there's no src/docker directory, you must be meant <rootDir/docker. File currently lives there and seems CI needs it from there (as same as <rootDir/docker/build.sh`, @chevdor

@chevdor I've checked Dockerfile, it use PROJECT_ROOT=$(git rev-parse --show-top-level) construct, so it will always use <rootDir> for building docker regardless to current $(pwd) it is running from.
I vote for keeping Dockerfile in <rootDir>/docker. You guys, @ddorgan @TriplEight ?

TriplEight · 2019-07-04T10:53:51Z

Currently, there's no Dockerfile in polkadot/docker dir on master. Please check polkadot/scripts/docker/Dockerfile, we use that one.
Besides, our security policy demands keeping our docker images on our own dockerhub repository, which is https://hub.docker.com/r/parity/polkadot/tags .
This dockerfile you've fixed suits local Polkadot builds, and we do that in CI as I described above.
I think polkadot/docker dir should be removed.

aahutsal · 2019-07-04T12:49:17Z

@TriplEight in scope of current project I have to create 2 Docker nodes with specific network configuration (one sits behind another). I'm going to use docker-compose for it and keep configuration inside polkadot/docker directory. So it can't be removed. If you check this pull request, you will find Dockerfile inside polkadot/docker directory. It's not merged to master yet, so you can't see it there.
I will compare recent polkadot/docker/Dockerfile with polkadot/scripts/docker/Dockerfile and make sure its identical.

chevdor · 2019-07-05T09:20:24Z

@ddorgan @TriplEight

In CI we copy the Dockerfile from scripts/docker/ to the dir with the compiled binary

I think the statement above is good for CI as it is quicker.

For users however, making the assumption that the binary is already built is not a good one. The goal of making a 2 stages images (beside making the image as small as possible) is to allow users with various OS and presence/absence of any toolchain to be able to build the docker image.

chevdor · 2019-07-05T09:24:00Z

@agutsal

@chevdor I've checked Dockerfile, it use PROJECT_ROOT=$(git rev-parse --show-top-level) construct, so it will always use for building docker regardless to current $(pwd) it is running from.

I don't like this 'trick' either, see paritytech/substrate#2464

aahutsal · 2019-07-05T10:50:35Z

@chevdor funny ;) We're discussing right Dockerfile placement for 4th day already ;)

I'll review and put my comments on it.

TriplEight · 2019-07-05T12:37:54Z

@chevdor
I understand that this way of building and running is important for some users. You are the only one who supports these configurations and this is valuable. We neither use nor support them. In addition, using docker images not hosted in our repository is not consistent with our internal security policy.

I suggest the following: you move these files and descriptions to your own repository, and I will refer to this in README.md as a community effort. Deal?

chevdor · 2019-07-05T15:40:25Z

@TriplEight thanks for your offer but I don't see it as a great deal for the community.

The explanation I would write here is very similar to this one: paritytech/substrate#1321 (comment)

In a nutshell, I think the new image you recently made is great for the CI, not so great for the users, in a decentralized landscape, who may want to build their own nodes. Many users, back up to a year ago, were able to get started with polkadot because this Dockerfile was available. The same Dockerfile they could later take over and build on.

As a matter of fact, you can see in various places that ppl build 'their' image based on root Dockerfile of the repo. This is very healthy for the ecosystem that people don't put too much trust in any one single source, that includes the official parity image.

If you think the current root image has issues, I think the best option would be to discuss and see how we can improve that. We may even find a solution where a single image would work for the 2 requirements. IMO that would be ideal and I am sure we can make it fit whatever is your internal security policy (which btw, I have no problem complying with if this is published somewhere).

In short, I think the change @agutsal is bringing is good (I would just suggest to leave the file where it was) and I am happy to see such contribution. @agutsal I would be happy to merge but I don't have such an access so I can only state my 👍 as soon as you move back.

If docker/script.sh needs adjustments, I think the best is to adapt there. I am not even sure we need this file in the end, it is there once again to make it simpler for users to build their own image.

aahutsal · 2019-07-08T17:41:55Z

@chevdor You probably meant docker/Dockerfile not docker/script.sh (there are not such file).
@TriplEight I've compared two Dockerfile(s) and must say that Dockerfile you currently use for CI require the code to be built on host system (that means all build deps/tools should be installed on host system which is not good), while docker/Dockerfile does this from inside first docker container, then just move built executable o 2nd container dropping 1st one. I think would be good if you just adopt your CI to use docker/Dockerfile I've just finalized in that PR. Thank you.

docker/Dockerfile

andresilva · 2019-07-10T15:37:04Z

docker/Dockerfile

+        export PATH=$PATH:$HOME/.cargo/bin && \
+        rustup toolchain install nightly && \
+        rustup target add wasm32-unknown-unknown --toolchain nightly && \
+        cargo install --git https://github.com/alexcrichton/wasm-gc && \


Can't all of these commands to setup the rust environment be replaced by a call to ./scripts/init.sh?

Trying. Will update you shortly

@agutsal you can reference (or even mention it in FROM) this one. We are updating it nightly.

@andresilva that init.sh is not used by us anywhere btw.

@andresilva @TriplEight I guess you guys should come to some agreement and not waste my time. Let me know by tagging me in that thread. I'll switch notifications off in the meantime. Thank you.

@agutsal I can't push to your branch otherwise I'd fix it myself. This works 060191b and I would merge it. I think you can allow this by clicking the allow edits from maintainers checkbox somewhere in this page.

@TriplEight it's not used by us but it's mentioned in the documentation of both substrate and polkadot. It's a small script that does pretty much the same thing that's being done in these lines of the Dockerfile. The intention of this PR is to fix the existing Dockerfile so let's keep the changes to that end.

@andresilva , ok then. I will check if it will work, a bit later.

@andresilva it was checked by default:

andresilva · 2019-07-12T12:01:30Z

I confirmed the Dockerfile works, I built an image with it. Thanks for the PR @agutsal.

* MQC auth Update polkadot WIP * Update polkadot * Silly syntax errors * Fix typo * Leave some comments and docs * Apply suggestions from code review Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Introduce the MessageQueueChain structure * Move the HRMP channel relevance check below * Fix the `receive_hrmp_after_pause` test * ValidationData is passed by reference * Replace "to cumulus" with "to the collator" * Update the test so that they are same as in polkadot Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Dockerfile fixed.

6bb8eed

aahutsal mentioned this pull request Jul 2, 2019

Create a sentry node configuration for Polkadot validators with guide w3f/General-Grants-Program#122

Closed

andresilva suggested changes Jul 10, 2019

View reviewed changes

andresilva added 2 commits July 11, 2019 16:22

docker: use init.sh to setup rust env

060191b

Merge branch 'master' into agutsal-master

b3e3d54

andresilva approved these changes Jul 12, 2019

View reviewed changes

andresilva merged commit 81645ec into paritytech:master Jul 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dockerfile fixed. #308

Dockerfile fixed. #308

aahutsal commented Jul 2, 2019

parity-cla-bot commented Jul 2, 2019

aahutsal commented Jul 2, 2019

parity-cla-bot commented Jul 2, 2019

aahutsal commented Jul 2, 2019

chevdor commented Jul 3, 2019

aahutsal commented Jul 3, 2019

ddorgan commented Jul 3, 2019

TriplEight commented Jul 3, 2019

aahutsal commented Jul 4, 2019

TriplEight commented Jul 4, 2019

aahutsal commented Jul 4, 2019

chevdor commented Jul 5, 2019

chevdor commented Jul 5, 2019

aahutsal commented Jul 5, 2019

TriplEight commented Jul 5, 2019

chevdor commented Jul 5, 2019

aahutsal commented Jul 8, 2019

andresilva Jul 10, 2019

aahutsal Jul 10, 2019

TriplEight Jul 11, 2019

TriplEight Jul 11, 2019

aahutsal Jul 11, 2019

andresilva Jul 11, 2019

TriplEight Jul 12, 2019

aahutsal Jul 12, 2019

andresilva commented Jul 12, 2019

Dockerfile fixed. #308

Dockerfile fixed. #308

Conversation

aahutsal commented Jul 2, 2019

parity-cla-bot commented Jul 2, 2019

aahutsal commented Jul 2, 2019

parity-cla-bot commented Jul 2, 2019

aahutsal commented Jul 2, 2019

chevdor commented Jul 3, 2019

aahutsal commented Jul 3, 2019

ddorgan commented Jul 3, 2019

TriplEight commented Jul 3, 2019

aahutsal commented Jul 4, 2019

TriplEight commented Jul 4, 2019

aahutsal commented Jul 4, 2019

chevdor commented Jul 5, 2019

chevdor commented Jul 5, 2019

aahutsal commented Jul 5, 2019

TriplEight commented Jul 5, 2019

chevdor commented Jul 5, 2019

aahutsal commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva commented Jul 12, 2019