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

Add workspace initial support #1358

Merged
merged 14 commits into from
Nov 6, 2023

Conversation

aon
Copy link
Contributor

@aon aon commented Oct 4, 2023

Summary

Closes #1357

  • [n] y/n | Does it introduce breaking changes?
  • [n] y/n | Is it dependent on the specific version of ink or pallet-contracts?

Adds unstable flag workspace-mode that enables the usage of cargo-contracts inside a workspace package.

Description

Initially, this PR aimed to address use-ink/ink#1919, but it also fixes the issue when running cargo-contract.

This PR fixes the issue by changing in the manifest all workspace-inherited dependencies to normal dependencies. This is done by grabbing the workspace definition, and merging the dependencies with the ones defined in the crate.

For example if the workspace Cargo.toml is:

# Cargo.toml
[workspace]
members = ["contract"]

[workspace.dependencies]
ink = { version = "4.3.0", default-features = false }

And the contract Cargo.toml is:

# contract/Cargo.toml
[package]
name = "contract"
version = "0.1.0"
edition = "2021"
authors = ["[your_name] <[your_email]>"]

[lib]
path = "lib.rs"

[features]
default = ["std"]
std = [
    "ink/std",
    "scale/std",
    "scale-info/std",
]
ink-as-dependency = []

[dependencies]
ink = { workspace = true, default-features = true }

Then when building, cargo-contract will use a Cargo.toml with merged dependencies:

# contract/Cargo.toml
[package]
name = "contract"
version = "0.1.0"
edition = "2021"
authors = ["[your_name] <[your_email]>"]

[lib]
path = "lib.rs"

[features]
default = ["std"]
std = [
    "ink/std",
    "scale/std",
    "scale-info/std",
]
ink-as-dependency = []

[dependencies]
ink = { version = "4.3.0", default-features = true }

Checklist before requesting a review

  • My code follows the style guidelines of this project
  • I have added an entry to CHANGELOG.md
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Oct 4, 2023

User @aon, please sign the CLA here.

@SkymanOne
Copy link
Contributor

SkymanOne commented Oct 5, 2023

We need to understand what repercussions it has on a crate publishing process. We've experienced some issues with the release process in ink! where come internal crates had cyclic dependencies

@ascjones
Copy link
Collaborator

ascjones commented Oct 5, 2023

We need to understand what repercussions it has on a crate publishing process. We've experienced some issues with the release process in ink! where come internal crates had cyclic dependencies

This PR is to do with user contract projects with workspace dependencies.

That said, this project itself could also be upgraded to use workspace dependencies, I think hernando started a PR for that somewhere.

@aon
Copy link
Contributor Author

aon commented Oct 5, 2023

I agree this should be the default behaviour, just wanted to add it for now as unstable given it's not proven yet how well it works.

I'm unsure on the part about cyclic dependencies and how could this small change affect the crate publishing process?

On another note, I believe the CI is failing but due to unrelated things.

@ascjones
Copy link
Collaborator

ascjones commented Oct 6, 2023

I agree this should be the default behaviour, just wanted to add it for now as unstable given it's not proven yet how well it works.

I'm fine with bringing it straight in, if it is well tested. i.e. works to compile all existing ink-examples, and with a workspace example. Maybe we should add an example workspace crate layout as part of this PR to test that it works.

I'm unsure on the part about cyclic dependencies and how could this small change affect the crate publishing process?

I believe @SkymanOne was referring to the issues we experienced when migrating ink itself to workspace dependencies. I don't think this is an issue for this PR since we are dealing with the target contract crate workspace.

On another note, I believe the CI is failing but due to unrelated things.

Should be fixed once #1352 is merged. Just merge in master.

@aon
Copy link
Contributor Author

aon commented Oct 6, 2023

@ascjones I have created a PR in ink-examples following your recommendation, showing how it works. use-ink/ink-examples#44

crates/build/src/workspace/manifest.rs Outdated Show resolved Hide resolved
crates/build/src/workspace/manifest.rs Show resolved Hide resolved
crates/build/src/workspace/manifest.rs Outdated Show resolved Hide resolved
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Oct 30, 2023

User @faculerena, please sign the CLA here.

@faculerena
Copy link
Contributor

Pull request comments addressed.

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 thanks!

@ascjones ascjones merged commit afddadd into use-ink:master Nov 6, 2023
10 checks passed
@valeriacaracciolo
Copy link

@ascjones I have created a PR in ink-examples following your recommendation, showing how it works. paritytech/ink-examples#44

We (@faculerena, @tenuki) closed this PR due to some problems with checks and opened PR paritytech/ink-examples#52 as a replacement.

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.

Build fails when running cargo-contract inside a workspace package
5 participants