-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
…les on github Signed-off-by: Denis S. Soldatov aka General-Beck <general.beck@gmail.com>
scripts/gitlab/build.sh
Outdated
@@ -1,7 +1,5 @@ | |||
#!/bin/bash |
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.
Are we even using this build.sh
monstrosity?
I think at least partially the idea was to replace it with smaller and more straightforward task-specific scripts (which it seems like you did). Also, build.sh
is never mentioned in the gitlab-ci.yml
scripts/gitlab/push-binaries.sh
Outdated
fi | ||
aws s3 rm --recursive s3://$S3_BUCKET/$CI_BUILD_REF_NAME/$BUILD_PLATFORM | ||
aws s3api put-object --bucket $S3_BUCKET --key $CI_BUILD_REF_NAME/$BUILD_PLATFORM/parity$S3WIN --body target/$PLATFORM/release/parity$S3WIN | ||
aws s3api put-object --bucket $S3_BUCKET --key $CI_BUILD_REF_NAME/$BUILD_PLATFORM/parity$S3WIN.md5 --body parity$S3WIN.md5 |
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.
Do we even build md5 in our new pipeline?
Let's just stop doing that, and stop caring about it?
sha256
is better in every possible way, let's just stick to it?
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.
Overall — looks quite good; I've leaf a couple of comments on some smaller issues above.
Okay, thanks @General-Beck. There is only one reasonable way to test this:
|
CI is all red |
@General-Beck Looks like |
@General-Beck is this up to date with master? |
Assuming you are working on this somewhere else (Gitlab?), I'll close this and we can reopen this once ready. Please keep me in the loop, happy to provide early feedback. |
.gitlab-ci.yml
Outdated
- scripts/gitlab-build.sh i686-unknown-linux-gnu i686-unknown-linux-gnu i386 gcc g++ linux | ||
tags: | ||
- rust-i686 | ||
- gitlab-next |
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.
Shouldn't this be removed before the merge?
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.
yes
.gitlab-ci.yml
Outdated
only: &publishable_branches | ||
- nightly # Our nightly builds from schedule, on `master` | ||
- "v*" # Our version tags | ||
- gitlab-next |
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.
Same here, seems to be a stub for testing, not something we need to merge.
.gitlab-ci.yml
Outdated
image: parity/rust:gitlab-ci | ||
- scripts/gitlab/publish-docker.sh parity-evm | ||
|
||
publish:github:s3: |
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.
This (and the corresponding script push.sh
) are named misleadingly. I can easily imagine us forgetting that we're pushing to both locations from the way this section and accompanying script are called.
Also: why don't we have pushing to S3 and pushing to Github as separate steps?
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.
+1 for separate steps if possible
.gitlab-ci.yml
Outdated
|
||
####stage: docs | ||
|
||
json:rpc:docs: |
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.
👏
|
||
build_docs() { | ||
echo "__________Build docs__________" | ||
npm install |
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.
Let's prefer yarn
everywhere, shouldn't we?
Dependencies caching should become much more straightforward with it.
scripts/gitlab/rpc-docs.sh
Outdated
commit_files() { | ||
echo "__________Commit files__________" | ||
git checkout -b rpcdoc-update-${CI_COMMIT_REF_NAME} | ||
git commit . |
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.
Isn't it intended to be git add
here?
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.
yes.
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.
LGMT, only minor grumbles. I'm in favor of merging this early and break stuff rather than keeping this around much longer :)
.gitlab-ci.yml
Outdated
- scripts/gitlab-build.sh i686-unknown-linux-gnu i686-unknown-linux-gnu i386 gcc g++ linux | ||
tags: | ||
- rust-i686 | ||
- gitlab-next |
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.
yes
.gitlab-ci.yml
Outdated
.publishable_branches: # list of git refs for publishing builds to the "production" locations | ||
only: &publishable_branches | ||
- nightly # Our nightly builds from schedule, on `master` | ||
- "v*" # Our version tags |
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.
branch vitalik-improvements
would match this, right? what about "v2*"
or even regex if possible?
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.
Great catch!
- tags | ||
- stable | ||
- triggers | ||
- master |
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.
does this mean tests are not run on pull requests anymore?
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.
Only tests with <<: *optional_test
included; testing with stable happens everywhere (see section test:rust:stable:
); testing with beta
and nightly
Rust versions are only limited to master branch. It makes some sense, but not the perfect one — probably we want to be sure that our code breaks on nightly
immediately, not during the next release.
.gitlab-ci.yml
Outdated
variables: | ||
CARGO_TARGET: armv7-linux-androideabi | ||
|
||
build:macos:x86_64: |
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.
for consistency, can we rename this to build:darwin:macos:x86_64
?
.gitlab-ci.yml
Outdated
windows: | ||
<<: *collect_artifacts | ||
|
||
build-windows-x86_64: |
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.
also build:windows:msvc:x86_64
?
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.
Unfortunately, windows builds can't have :
characters in their names, because windows paths can't have said character.
Maybe let's come up with a better (and universal) separator?
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.
Ok let's go back to -
then
.gitlab-ci.yml
Outdated
image: parity/rust:gitlab-ci | ||
- scripts/gitlab/publish-docker.sh parity-evm | ||
|
||
publish:github:s3: |
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.
+1 for separate steps if possible
|
||
If you continue, Parity will be installed as a user service. You will be able to use the Parity Wallet through your browser by using the menu bar icon, following the shortcut in the Launchpad or navigating to http://localhost:8180/ in your browser. | ||
|
||
Parity is distributed under the terms of the GPL." |
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.
What is this?
scripts/gitlab/publish-snap.sh
Outdated
set -u # treat unset variables as error | ||
|
||
case ${CI_COMMIT_REF_NAME} in | ||
master|*v2.1*|gitlab-next) export CHANNEL="edge";; |
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.
replace gitlab-next
with 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.
maybe it's sufficient on nightly
only, not master
scripts/gitlab/rpc-docs.sh
Outdated
commit_files() { | ||
echo "__________Commit files__________" | ||
git checkout -b rpcdoc-update-${CI_COMMIT_REF_NAME} | ||
git commit . |
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.
yes.
|
||
upload_files() { | ||
echo "__________Upload files__________" | ||
git push --tags |
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.
Does this include #9219 ?
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.
yes
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.
good job, let's give it a try.
curl https://sh.rustup.rs -sSf | sh -s -- -y && \ | ||
# rustup directory | ||
PATH=/root/.cargo/bin:$PATH && \ | ||
RUN apt update && apt install -y --no-install-recommends openssl libudev-dev file |
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.
Why libudev-dev? Shouldn't it be libudev?
And there is no dependency on libssl anymore, so why openssl?
cp -v ethkey $SNAPCRAFT_PART_INSTALL/usr/bin/ethkey | ||
cp -v ethstore $SNAPCRAFT_PART_INSTALL/usr/bin/ethstore | ||
cp -v whisper $SNAPCRAFT_PART_INSTALL/usr/bin/whisper | ||
stage-packages: [libc6, libssl1.0.0, libudev1, libstdc++6, cmake] |
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.
https://docs.snapcraft.io/build-snaps/syntax#stage-packages
so the files from these packages end up in the snap. if this is required it should be the same for the docker image.
- parity.zip | ||
name: "x86_64-apple-darwin_parity" | ||
windows: | ||
<<: *collect_artifacts |
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.
the previous jobs (e.g. linux:linux:android:armhf) get their *collect_artifacts block from the *build anchor, doesn't that also work for this one?
Addressed most of the issues. Will merge once the CI is green. And then we can work from there. |
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.
@5chdn Will this master
→nightly
change do what you actually need? If yes, please merge at will.
.gitlab-ci.yml
Outdated
<<: *publish_docker | ||
script: | ||
- scripts/gitlab/publish-docker.sh parity-evm | ||
|
||
publish:github:s3: | ||
publish-github-and-s3: |
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.
In my ideal world we would split this into two separate steps.
But let's merge it as it is for now.
scripts/gitlab/package-snap.sh
Outdated
@@ -3,7 +3,7 @@ | |||
set -e # fail on any error | |||
set -u # treat unset variables as error | |||
case ${CI_COMMIT_REF_NAME} in | |||
master|*v2.1*|gitlab-next) export GRADE="devel";; | |||
nightly|*v2.1*) export GRADE="devel";; |
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.
Are we going to publish nightlies only from the special nightly
branch? Why not from the master
?
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.
master is triggered always after we merge a pull request, so multiple times per day and nightly is only happing once per night which is sufficient. now, master does not create a snap which is fine IMHO.
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.
@5chdn Alternatively, we can avoid triggering the actual build-and-package on master
on every push (only tests), and trigger the builds only once per day with a cron-like trigger (Gitlab CI already supports those). This way, there would be no need in maintaining a separate nightly
branch manually.
If you agree with me that not having to maintain separate nightly
is desired, please open a separate issue for this (or let me know, and I'll do this).
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.
we are doing that. nightly is a tag.
🎉 |
* master: evmbin: Fix gas_used issue in state root mismatch and handle output better (#9418) Update hardcoded sync (#9421) Add block reward contract config to ethash and allow off-chain contracts (#9312) Private packets verification and queue refactoring (#8715) Update tobalaba.json (#9419) docs: add parity ethereum logo to readme (#9415) build: update rocksdb crate (#9414) Updating the CI system (#8765) Better support for eth_getLogs in light mode (#9186) Add update docs script to CI (#9219) `gasleft` extern implemented for WASM runtime (kip-6) (#9357) block view! removal in progress (#9397) Prevent sync restart if import queue full (#9381) nonroot CentOS Docker image (#9280) ethcore: kovan: delay activation of strict score validation (#9406)
Updating the CI system with the publication of releases and binary files on github. example
Removed i686 and armv6 builds.
Removed md5 hash.
Added parity/parity-evm Docker image.
After each binary file is builded for a given architecture, they are automatically packaged for different installers.
Artifacts are created at each stage and stored for a month on the server.
binaries from build stage
packages
List of generated systems and packages:
Improved build time with the caching cargo.
Need to test on
nightly
andtags
.We will need to change the vanity to add files to the download section.
and many more changes...
P.S. now everything has become beautiful!