Workspace dependencies and properties #1835

pmikolajczyk41 · 2023-07-04T13:04:28Z

closes: #1814

Notes:

we had to use default-features = false in workspace (workspaces.dependencies causes ignore of default-features = false in crate rust-lang/cargo#11329)
scale (alias for parity-scale-codec) was left in every Cargo.toml due to: Does not support workspace inheriting bkchr/proc-macro-crate#34; solved similarly to subxt and polkadot-introspector repositories

.gitignore

pmikolajczyk41 · 2023-07-05T11:29:28Z

classically we are struggling with bkchr/proc-macro-crate#34

xgreenx · 2023-07-16T09:36:21Z

crates/engine/Cargo.toml


 [dependencies]
-ink_primitives = { version = "4.2.0", path = "../../crates/primitives", default-features = false }
+derive_more = { workspace = true, features = ["from", "display"] }
+# cannot inherit due to: /~https://github.com/bkchr/proc-macro-crate/issues/34
 scale = { package = "parity-scale-codec", version = "3.4", default-features = false, features = ["derive"] }


You can define this alias on the workspace level and use here scale = { workspace = true }

please check my comment: #1835 (comment)

Works for me:

have you run the code? I have done it some time ago, but this doesn't compile: 08c1937

(macro generation phase fails)

btw, package is ignored when workspace is present

/~https://github.com/pmikolajczyk41/ink/pull/1/files

I will ensure that CI works well now and if so, I will apply these changes

crates/env/Cargo.toml

xgreenx · 2023-07-16T09:41:50Z

crates/ink/Cargo.toml

+repository.workspace = true
+version.workspace = true
+
+authors = ["Parity Technologies <admin@parity.io>", "Robin Freyler <robin@parity.io>"]


It seems authors are the same, maybe we can use workspace too?

no, they are not the same across crates and I really don't want to get into authorship resolution here - I don't know what is the legal status of it

Only 10 of 57 are unique. For them, you can use a custom variant

there are 14 crates in workspace, only 3 of them have this basic authorship; the rest that you have grepped are in integration-tests; nevertheless, suggestion applied

Hmm, maybe you forgot to push or maybe I'm reviewing too earlier=D

yes, that's the case, sorry

xgreenx · 2023-07-16T09:44:09Z

crates/ink/Cargo.toml

-ink_prelude = { version = "4.2.0", path = "../prelude", default-features = false }
-ink_macro = { version = "4.2.0", path = "macro", default-features = false }
-
+derive_more = { workspace = true, features = ["from"] }


Could you keep the order the same for now, please? Let's change it in a separate PR. We can use cargo-sort and include it in our CI

all done in-place

xgreenx · 2023-07-16T09:52:00Z

crates/ink/codegen/Cargo.toml


-serde = { version = "1.0.137", default-features = false, features = ["derive"] }
-serde_json = "1.0.81"
+ink_ir = { workspace = true }


I think returning the alias ir back, for now, is better to avoid conflicts. Later we can change that in a separate PR

leaving the old version then

xgreenx

Could you merge master please? Sorry, we added some conflicts to you

xgreenx · 2023-07-18T21:38:15Z

crates/engine/Cargo.toml


 [dependencies]
-ink_primitives = { version = "4.2.0", path = "../../crates/primitives", default-features = false }
+derive_more = { workspace = true, features = ["from", "display"] }
+# cannot inherit due to: /~https://github.com/bkchr/proc-macro-crate/issues/34
 scale = { package = "parity-scale-codec", version = "3.4", default-features = false, features = ["derive"] }


/~https://github.com/pmikolajczyk41/ink/pull/1/files

crates/env/Cargo.toml

xgreenx · 2023-07-18T21:45:39Z

crates/ink/Cargo.toml

+repository.workspace = true
+version.workspace = true
+
+authors = ["Parity Technologies <admin@parity.io>", "Robin Freyler <robin@parity.io>"]


Hmm, maybe you forgot to push or maybe I'm reviewing too earlier=D

pmikolajczyk41 · 2023-07-18T22:02:35Z

@xgreenx I'm force pushing to have a cleaner start - lmk if the only outstanding issues are with parity-scale-codec and ink_ir alias in one crate

This reverts commit af38b3a.

pmikolajczyk41 · 2023-07-19T01:52:33Z

it seems that the incorrect remedy for bkchr/proc-macro-crate#34 is to add package attribute to scale = {workspace = true} dependency, which actually is ignored by rust but is not ignored by the proc-macro-crate library; it's kinda bug, but it works

the cost is the list of warnings during compilation

pmikolajczyk41 · 2023-07-21T00:21:09Z

@xgreenx @ascjones can you please take a look at the current state?

SkymanOne · 2023-07-21T11:00:40Z

Overall looks good to me. However, I've noticed a warning during cargo contract build and cargo test:

warning: /Users/skyman/Projects/ink/crates/ink/Cargo.toml: unused manifest key: dependencies.scale.package

pmikolajczyk41 · 2023-07-21T14:50:57Z

@SkymanOne it's the thing described here: #1835 (comment)

SkymanOne · 2023-07-24T10:39:21Z

crates/allocator/Cargo.toml

@@ -1,25 +1,25 @@
 [package]
 name = "ink_allocator"
-version = "4.2.0"
+version.workspace = true
 authors = ["Parity Technologies <admin@parity.io>", "Robin Freyler <robin@parity.io>"]


Just a suggestion for @cmichi and @ascjones : Should we inherit authors from the workspace too? It appears that it should consistently be "Parity Technologies <admin@parity.io>"

See discussion above. @Robbepop is the original author of some of the crates so those ones will remain not inhherited, and the rest inherit the default.

xgreenx

Thank you for addressing all comments, much easier to review=)

ascjones

LGTM, Sorry I just merged some dependency bumps so you will need to resolve those conflicts then we can merge.

# Conflicts: # crates/e2e/Cargo.toml # crates/e2e/macro/Cargo.toml # crates/metadata/Cargo.toml

pmikolajczyk41 · 2023-07-25T10:41:58Z

@ascjones it should be now ready to go :)

deuszx · 2023-07-25T21:11:54Z

Cargo.toml

+ink_env = { version = "4.2.0", path = "crates/env", default-features = false }
+ink_ir = { version = "4.2.0", path = "crates/ink/ir", default-features = false }


Why are these two not fixed?

thanks for spotting: #1856

pmikolajczyk41 requested review from cmichi, ascjones and SkymanOne as code owners July 4, 2023 13:04

ascjones reviewed Jul 4, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

xgreenx reviewed Jul 16, 2023

View reviewed changes

xgreenx mentioned this pull request Jul 16, 2023

Pin inter crate dependencies to specific version #1813

Merged

Workspace properties changed in-place

c5fffc5

xgreenx reviewed Jul 18, 2023

View reviewed changes

pmikolajczyk41 added 3 commits July 18, 2023 14:46

Workspace dependencies changed in-place

226d634

, default-features = true

557b6fa

pin

370d82b

pmikolajczyk41 force-pushed the pmikolajczyk41/1814 branch from 574f5f2 to 370d82b Compare July 18, 2023 22:02

pmikolajczyk41 added 5 commits July 18, 2023 15:26

ui tests

71c17df

Newline in ui-test

ae37585

scale

2d79a2f

Remove package attribute

af38b3a

Revert "Remove package attribute"

6df6d00

This reverts commit af38b3a.

pmikolajczyk41 mentioned this pull request Jul 19, 2023

Does not support workspace inheriting bkchr/proc-macro-crate#34

Closed

SkymanOne reviewed Jul 24, 2023

View reviewed changes

xgreenx approved these changes Jul 24, 2023

View reviewed changes

ascjones approved these changes Jul 25, 2023

View reviewed changes

pmikolajczyk41 added 2 commits July 25, 2023 12:10

Merge remote-tracking branch 'origin/master' into pmikolajczyk41/1814

fe27c05

# Conflicts: # crates/e2e/Cargo.toml # crates/e2e/macro/Cargo.toml # crates/metadata/Cargo.toml

Update

4a26141

ascjones merged commit ded873f into use-ink:master Jul 25, 2023

pmikolajczyk41 deleted the pmikolajczyk41/1814 branch July 25, 2023 11:22

deuszx reviewed Jul 25, 2023

View reviewed changes

pmikolajczyk41 mentioned this pull request Jul 26, 2023

Fix ink dependency versions #1856

Merged

pmikolajczyk41 mentioned this pull request Aug 14, 2023

v0.1.0: ready to replace existing substrate subcommand paritytech/try-runtime-cli#6

Merged

16 tasks

pmikolajczyk41 mentioned this pull request Oct 3, 2023

Get rid of the cargo warning about unused manifest key #1929

Closed

SkymanOne mentioned this pull request Mar 4, 2024

Release 5.0.0 #2133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workspace dependencies and properties #1835

Workspace dependencies and properties #1835

pmikolajczyk41 commented Jul 4, 2023 •

edited

Loading

pmikolajczyk41 commented Jul 5, 2023

xgreenx Jul 16, 2023

pmikolajczyk41 Jul 16, 2023

xgreenx Jul 16, 2023

pmikolajczyk41 Jul 18, 2023

pmikolajczyk41 Jul 18, 2023

xgreenx Jul 18, 2023

pmikolajczyk41 Jul 18, 2023

xgreenx Jul 16, 2023

pmikolajczyk41 Jul 16, 2023

xgreenx Jul 16, 2023

pmikolajczyk41 Jul 18, 2023

xgreenx Jul 18, 2023

pmikolajczyk41 Jul 18, 2023

pmikolajczyk41 Jul 18, 2023

xgreenx Jul 16, 2023

pmikolajczyk41 Jul 18, 2023

xgreenx Jul 16, 2023

pmikolajczyk41 Jul 19, 2023

xgreenx left a comment

xgreenx Jul 18, 2023

xgreenx Jul 18, 2023

pmikolajczyk41 commented Jul 18, 2023

pmikolajczyk41 commented Jul 19, 2023 •

edited

Loading

pmikolajczyk41 commented Jul 21, 2023

SkymanOne commented Jul 21, 2023

pmikolajczyk41 commented Jul 21, 2023

SkymanOne Jul 24, 2023

ascjones Jul 25, 2023

xgreenx left a comment

ascjones left a comment

pmikolajczyk41 commented Jul 25, 2023

deuszx Jul 25, 2023

pmikolajczyk41 Jul 26, 2023

		ink_env = { version = "4.2.0", path = "crates/env", default-features = false }
		ink_ir = { version = "4.2.0", path = "crates/ink/ir", default-features = false }

Workspace dependencies and properties #1835

Workspace dependencies and properties #1835

Conversation

pmikolajczyk41 commented Jul 4, 2023 • edited Loading

pmikolajczyk41 commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmikolajczyk41 commented Jul 18, 2023

pmikolajczyk41 commented Jul 19, 2023 • edited Loading

pmikolajczyk41 commented Jul 21, 2023

SkymanOne commented Jul 21, 2023

pmikolajczyk41 commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

pmikolajczyk41 commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmikolajczyk41 commented Jul 4, 2023 •

edited

Loading

pmikolajczyk41 commented Jul 19, 2023 •

edited

Loading