Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build contracts and dylint driver with stable #698

Merged
merged 11 commits into from
Aug 23, 2022
Merged

Build contracts and dylint driver with stable #698

merged 11 commits into from
Aug 23, 2022

Conversation

athei
Copy link
Contributor

@athei athei commented Aug 20, 2022

Instead of using a nightly toolchain we want to build contracts with a stable compiler. Since we use nightly features we need to pass the RUSTC_BOOTSTRAP=1 env variable.

This PR does the following:

  • Pass RUSTC_BOOTSTRAP=1 when building contracts and the dylint driver
  • Check that the user uses a stable compiler with a minimum version
  • Remove the toolchain file from ink_dylint since we just build it with a stable compiler
    • The UI tests in CI are still run against a fixed nightly version
  • Fix the CI to use a stable compiler and other small stuff that broke

Future Work

I don't like this brittle setup of packaging the dylint binary driver at call. I suggest we just patch the users manifest on the fly at runtime to contain this:

[workspace.metadata.dylint]
libraries = [
    { git = "https://github.com/paritytech/cargo-contract/", branch = "$cargo-contract-version", pattern = "ink_dylint/" },
]

This will remove all this horror of the build.rs script and also removes the need for this horrible _Cargo.toml rename. We just use dylint as it is supposed to be used. Also, this doesn't force the user to run the dylint on the toolchain the driver was build but the one they compile their contract with anyways. And before you ask: The driver compiles quickly and is obviously cached in the users target dir.

@athei athei marked this pull request as ready for review August 20, 2022 10:38
@athei athei requested review from a team, cmichi, ascjones and HCastano as code owners August 20, 2022 10:38
@xgreenx
Copy link
Collaborator

xgreenx commented Aug 20, 2022

I tried to install the cargo-contract from this branch and I got the follow errors:

error: failed to run custom build command for `cargo-contract v1.5.0 (/Users/green/External/4ire.labs/cargo-contract)`

Caused by:
  process didn't exit successfully: `/Users/green/cache/target/release/build/cargo-contract-a7b1b1d20aac2f32/build-script-build` (exit status: 1)
  --- stdout
  cargo:rustc-env=CARGO_CONTRACT_CLI_IMPL_VERSION=1.5.0-4f5db94-x86_64-apple-darwin
  cargo:rerun-if-changed=/Users/green/External/4ire.labs/cargo-contract/.git/HEAD
  cargo:rerun-if-changed=/Users/green/External/4ire.labs/cargo-contract/.git/refs/heads/at/stable

  Creating template zip: template_dir '/Users/green/External/4ire.labs/cargo-contract/templates/new', destination archive '/Users/green/cache/target/release/build/cargo-contract-25f37fd80301513f/out/template.zip'
  Done: /Users/green/External/4ire.labs/cargo-contract/templates/new written to /Users/green/cache/target/release/build/cargo-contract-25f37fd80301513f/out/template.zip
  Setting cargo working dir to '/Users/green/External/4ire.labs/cargo-contract/ink_linting'
  Invoking cargo: "cargo" "build" "--release" "--locked" "--target-dir=/Users/green/cache/target/release/build/cargo-contract-25f37fd80301513f/out" "--manifest-path=/Users/green/External/4ire.labs/cargo-contract/ink_linting/Cargo.toml"

  --- stderr
  warning: skipping duplicate package `fail-both-same` found at `/Users/green/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_rust_version/fail_both_same`
  warning: skipping duplicate package `fail-cargo` found at `/Users/green/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_rust_version/fail_cargo`
  warning: skipping duplicate package `fail-file-attr` found at `/Users/green/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_rust_version/fail_file_attr`
  warning: skipping duplicate package `fail-clippy` found at `/Users/green/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_rust_version/fail_clippy`
  warning: skipping duplicate package `fail` found at `/Users/green/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/module_style/pass_mod`
  warning: skipping duplicate package `fail` found at `/Users/green/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/module_style/fail_mod`
  warning: skipping duplicate package `cargo_common_metadata` found at `/Users/green/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_common_metadata/fail_publish_true`
     Compiling clippy_utils v0.1.64 (https://github.com/rust-lang/rust-clippy?rev=0cb0f7636851f9fcc57085cf80197a2ef6db098f#0cb0f763)
  error[E0530]: match bindings cannot shadow tuple variants
     --> /Users/green/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/clippy_utils/src/ast_utils.rs:603:25
      |
  601 |     use Extern::*;
      |         --------- the tuple variant `Implicit` is imported here
  602 |     match (l, r) {
  603 |         (None, None) | (Implicit, Implicit) => true,
      |                         ^^^^^^^^
      |                         |
      |                         cannot be named the same as a tuple variant
      |                         help: try specify the pattern arguments: `Implicit(..)`

  error[E0416]: identifier `Implicit` is bound more than once in the same pattern
     --> /Users/green/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/clippy_utils/src/ast_utils.rs:603:35
      |
  603 |         (None, None) | (Implicit, Implicit) => true,
      |                                   ^^^^^^^^ used in a pattern more than once

  error[E0408]: variable `Implicit` is not bound in all patterns

The command cargo +stable install --path ".".

Do you have any idea what is the problem?=D

@athei
Copy link
Contributor Author

athei commented Aug 20, 2022

My best guess is your stable compiler is not up to date.

@xgreenx
Copy link
Collaborator

xgreenx commented Aug 20, 2022

It was the newest one, but I don't know what the problem was. I removed the toolchain and installed it again with a stable default toolchain. I get an error again, but after installing all components again, it works)

Cool, I still can build contracts with #![feature(min_specialization)]=D

I can't build tests with stable(because I run them throw cargo test and bootstrap is not enabled). And I realized that I use the sha2-const crate in a new storage refactoring. And this crate requires #![feature(const_mut_refs)] feature. So I can't build tests with stable rust=(

@athei
Copy link
Contributor Author

athei commented Aug 20, 2022

What do you expect? Run cargo contract test. Not flat cargo test.

@ascjones
Copy link
Collaborator

I don't like this brittle setup of packaging the dylint binary driver at call. I suggest we just patch the users manifest on the fly.

I agree, this should be relatively straightforward, the one slight possible complication would be involving:

The driver compiles quickly and is obviously cached in the users target dir.

Because we build the contract in a temp directory (with the modified manifest), and we set the --target-dir to the original dir, that's how the artifacts are preserved across builds. I have not tested it yet, but I had a quick look a few weeks ago, and from what I could tell the cargo dylint invocation would leave the artifacts in the tmp dir so they would be blown away. Since there is no --target-dir flag for cargo dylint, the solution might be to simply copy the target/dylint directory to and from the tmp dir before and after the contract build. Again I have not tried it out so this is speculation.

@athei
Copy link
Contributor Author

athei commented Aug 22, 2022

Because we build the contract in a temp directory (with the modified manifest), and we set the --target-dir to the original dir, that's how the artifacts are preserved across builds. I have not tested it yet, but I had a quick look a few weeks ago, and from what I could tell the cargo dylint invocation would leave the artifacts in the tmp dir so they would be blown away. Since there is no --target-dir flag for cargo dylint, the solution might be to simply copy the target/dylint directory to and from the tmp dir before and after the contract build. Again I have not tried it out so this is speculation.

I already have a branch where this is working. You can just pass the target dir via an env variable to cargo dylint. I will open this PR after this is in.

Copy link
Contributor

@HCastano HCastano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

src/util.rs Show resolved Hide resolved
src/util.rs Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
ink_linting/README.md Show resolved Hide resolved
.gitlab-ci.yml Show resolved Hide resolved
@@ -21,7 +21,7 @@ variables:
RUSTY_CACHIER_SINGLE_BRANCH: master
RUSTY_CACHIER_DONT_OPERATE_ON_MAIN_BRANCH: "true"
# paritytech/contracts-ci-linux:production defaults to nightly toolchain, default rusty-cachier to it too
RUSTY_CACHIER_TOOLCHAIN: nightly
RUSTY_CACHIER_TOOLCHAIN: stable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to update the Docker image in paritytech/scripts after this, eh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what this env variable actually does. It didn't change the toolchain being used to compile because that needed to be changed at other places.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is used by CI tool rusty-cachier which provides reusable Rust cache for Cargo.

If should be RUSTY_CACHIER_TOOLCHAIN: nightly for all jobs which use nightly toolchain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is used by CI tool rusty-cachier which provides reusable Rust cache for Cargo.

I figured that much but what does this actually mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to update the Docker image in paritytech/scripts after this, eh

Yes, the default toolchain for the CI image is nightly. But it could be overridden in the before_script: section as it was done below.

I figured that much but what does this actually mean?

rusty-cachier related environment variables are described here. RUSTY_CACHIER_TOOLCHAIN is used to select the toolchain to construct a cache key and defaults to stable. Toolchain version is used as a part of the cache key because different Rust versions have incompatible cargo_target_dir caches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all pipeline jobs are going to use the stable toolchain, the variable could be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was more referring to the line with paritytech/contracts-ci-linux:production (which wasn't touched in this PR).

@rcny not all jobs are going to use the same toolchain, some are still gonna use nightly. What should we do in this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add RUSTY_CACHIER_TOOLCHAIN: nightly under the variables: keyword at the specific jobs' level.

src/util.rs Outdated Show resolved Hide resolved
athei and others added 4 commits August 23, 2022 11:24
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

.gitlab-ci.yml Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants