-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Conversation
It looks like @agutsal hasn't signed our Contributor License Agreement, yet.
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 Many thanks, Parity Technologies CLA Bot |
[clabot:check] |
It looks like @agutsal signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
@chevdor seems I've fixed Dockerfile. It's moved to |
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! |
This will require to change |
@TriplEight is there a reason to keep this? Since we're building docker images on each master merge. |
@ddorgan this is deprecated. |
@TriplEight @ddorgan there's no @chevdor I've checked Dockerfile, it use |
Currently, there's no Dockerfile in |
@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 |
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. |
@agutsal
I don't like this 'trick' either, see paritytech/substrate#2464 |
@chevdor funny ;) We're discussing right I'll review and put my comments on it. |
@chevdor 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? |
@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 |
@chevdor You probably meant |
docker/Dockerfile
Outdated
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 && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't all of these commands to setup the rust environment be replaced by a call to ./scripts/init.sh
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying. Will update you shortly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agutsal you can reference (or even mention it in FROM
) this one. We are updating it nightly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andresilva that init.sh
is not used by us anywhere btw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andresilva , ok then. I will check if it will work, a bit later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andresilva it was checked by default:
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>
docker/build.sh
works now.