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

Bring test-ci-only back #180

Merged
merged 26 commits into from
Mar 10, 2021
Merged

Bring test-ci-only back #180

merged 26 commits into from
Mar 10, 2021

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Feb 18, 2021

We actually temporarily removed testing our CI with the feature test-ci-only in https://github.com/paritytech/cargo-contract/pull/114/files.

@cmichi cmichi marked this pull request as ready for review March 10, 2021 09:21
@cmichi cmichi requested a review from ascjones March 10, 2021 09:21
@@ -64,7 +64,7 @@ where
Verbosity::Default => &mut cmd,
};

log::info!("invoking cargo: {:?}", cmd);
log::info!("Invoking cargo: {:?}", cmd);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just for consistency with the other log messages, which start uppercase.

@@ -430,6 +430,9 @@ pub(crate) fn execute_with_crate_metadata(
"Building cargo project".bright_green().bold()
);
build_cargo_project(&crate_metadata, build_artifact, verbosity, unstable_flags)?;
if build_artifact == BuildArtifacts::CheckOnly {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this from some other PR?

@@ -315,7 +315,7 @@ enum Command {
/// Command has been deprecated, use `cargo contract build` instead
#[structopt(name = "generate-metadata")]
GenerateMetadata {},
/// Check that the code builds as Wasm; does not output any build artifact to the top level `target/` directory
/// Check that the code builds as Wasm; does not output any `<name>.contract` artifact to the `target/` directory
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We invoke cargo check with --target-dir. This actually results in a target folder in the target-dir.

--target-dir directory
Directory for all generated artifacts and intermediate files.

You can verify it locally:

$ cargo new foo
     Created binary (application) `foo` package
$ cd foo
$ cargo check
ls
    Checking foo v0.1.0 (/tmp/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 2.10s
$ ls
Cargo.lock  Cargo.toml  src  target

@ascjones
Copy link
Collaborator

Does this need merging with master?

@@ -466,30 +471,33 @@ mod tests_ci_only {
ManifestPath::new(&path.join("new_project").join("Cargo.toml")).unwrap();
let res = super::execute(
&manifest_path,
None,
Verbosity::Default,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a required change, actually the code does not build without it. We just didn't notice because the test-ci-only feature was not included in the CI.

@@ -430,6 +430,9 @@ pub(crate) fn execute_with_crate_metadata(
"Building cargo project".bright_green().bold()
);
build_cargo_project(&crate_metadata, build_artifact, verbosity, unstable_flags)?;
if build_artifact == BuildArtifacts::CheckOnly {
return Ok((None, None));
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This early return is needed because the next step would otherwise try to post-process the contract. But check does not actually create a contract.

Without this change cargo contract check fails with

 [2/2] Post processing wasm file
ERROR: Loading original wasm file '/tmp/foobar/target/ink/wasm32-unknown-unknown/release/foobar.wasm'

Caused by:
    Can't read from the file: Os { code: 2, kind: NotFound, message: "No such file or directory" }

ascjones and others added 5 commits March 10, 2021 15:20
…219)

* Only allow contract names beginning with an alphabetic character

* Add test for contract name beginning with a number

* Add test for contract name beginning with a number
…h/cargo-contract into cmichi-bring-test-ci-only-back
This reverts commit defe20d.
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

@ascjones ascjones merged commit 4111385 into master Mar 10, 2021
@ascjones ascjones deleted the cmichi-bring-test-ci-only-back branch March 10, 2021 15:17
@cmichi cmichi mentioned this pull request Mar 31, 2021
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.

2 participants