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

feat(crates-io): publish gstd and gear-wasm-builder #3515

Merged
merged 40 commits into from
Dec 5, 2023
Merged

Conversation

clearloop
Copy link
Contributor

@clearloop clearloop commented Nov 21, 2023

Resolves #3500
Resolves #3528
Resolves #3131

  • split crates-io-manager into parts
  • update parity-wasm in pwasm-utils ( -> gear-pwasm-utils )
  • publish gstd
  • publish gear-wasm-builder

testing action: https://github.com/gear-tech/gear/actions/runs/6970605696/job/18968871282

@gear-tech/dev

@clearloop clearloop changed the title feat(crates-io): publish packages on merging main feat(crates-io): publish packages on merging master Nov 21, 2023
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

why do we need publish it on each commit? that's meaningless. we must do it only on releases

@clearloop
Copy link
Contributor Author

why do we need publish it on each commit? that's meaningless. we must do it only on releases

the version is not bumping but with commit hash https://crates.io/crates/actor-system-error/1.0.2-354d660.8, btw since we are bumping versions before release, we can not run publishing tests with CI ( because --dry-run requires all dependencies being published ), the overall logic is easy to be broken once we add more local dependencies to the to-be-published packages

@clearloop clearloop added the A0-pleasereview PR is ready to be reviewed by the team label Nov 23, 2023
@clearloop clearloop changed the title feat(crates-io): publish packages on merging master feat(crates-io): publish gsdk and gear-wasm-builder Nov 23, 2023
@StackOverflowExcept1on
Copy link
Member

I think we can remove it now (parity-wasm, wasm-instrument) and use the versions from crates.io:

gear/Cargo.toml

Lines 501 to 504 in 3a06def

[patch.crates-io]
parity-wasm = { version = "0.45.0", git = "https://github.com/gear-tech/parity-wasm", branch = "v0.45.0-sign-ext" }
wasmi-validation = { version = "0.5.0", git = "https://github.com/gear-tech/wasmi", branch = "v0.13.2-sign-ext" }
wasm-instrument = { version = "0.3.0", git = "https://github.com/gear-tech/wasm-instrument", branch = "v0.3.0-sign-ext" }

Cargo.toml Outdated Show resolved Hide resolved
@clearloop clearloop marked this pull request as ready for review November 23, 2023 20:48
@clearloop clearloop changed the title feat(crates-io): publish gsdk and gear-wasm-builder feat(crates-io): publish gstd and gear-wasm-builder Nov 26, 2023
@breathx
Copy link
Member

breathx commented Nov 26, 2023

why do we need publish it on each commit? that's meaningless. we must do it only on releases

the version is not bumping but with commit hash https://crates.io/crates/actor-system-error/1.0.2-354d660.8, btw since we are bumping versions before release, we can not run publishing tests with CI ( because --dry-run requires all dependencies being published ), the overall logic is easy to be broken once we add more local dependencies to the to-be-published packages

So please write comment about it in each to-be-published crates toml. Also here's space to write some tooling that will check it.

We bump after releases so master always has greater version than last release. Also I couldn't find dry-run across repo.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
gcore/Cargo.toml Outdated Show resolved Hide resolved
gstd/Cargo.toml Outdated Show resolved Hide resolved
gstd/Cargo.toml Outdated Show resolved Hide resolved
utils/crates-io/src/lib.rs Outdated Show resolved Hide resolved
@clearloop
Copy link
Contributor Author

what about #3515 (comment)?

see the chat in our tg channel

@NikVolf
Copy link
Member

NikVolf commented Nov 27, 2023

@thecaralice care to take a look?

@thecaralice
Copy link
Contributor

rust-lang/cargo#1169 has been open for eight years now and still doesn't even have a draft implementation PR, so I don't think it will be implemented anytime soon. However, the discussion mentions different solutions being:

The latter looks pretty promising.

Note: looks like cargo publish --dry-run does not check versions (rust-lang/cargo#1169 (comment))

@thecaralice
Copy link
Contributor

What I dislike about this specific PR is that it doesn't have a single issue it addresses. I believe it should be split into several PRs:

  • Add cargo publish workflow job for automatic publishing on tags
  • Publish gstd and gear-wasm-builder
  • Update dependencies in pwasm-utils
  • Refactor crates.io manager (can questionably be merged with the first one)

@shamilsan shamilsan added the B1-releasenotes The feature deserves to be added to the Release Notes label Nov 28, 2023
Copy link
Contributor

@shamilsan shamilsan left a comment

Choose a reason for hiding this comment

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

👍

utils/crates-io/src/manifest.rs Show resolved Hide resolved
utils/crates-io/src/publisher.rs Outdated Show resolved Hide resolved
utils/crates-io/src/publisher.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
galloc/Cargo.toml Outdated Show resolved Hide resolved
gstd/codegen/Cargo.toml Outdated Show resolved Hide resolved
utils/crates-io/src/manifest.rs Outdated Show resolved Hide resolved
@clearloop
Copy link
Contributor Author

these tools don't work for our situation, bcz we need to modify specified dependencies before publishing them https://github.com/gear-tech/gear/blob/cl/issue-3500/utils/crates-io/src/rename.rs

Note: looks like cargo publish --dry-run does not check versions (rust-lang/cargo#1169 (comment))

we are not using --dry-run now to check publishing since our version in git repo is always greater than the release version

  • Add cargo publish workflow job for automatic publishing on tags

#3158

  • Publish gstd and gear-wasm-builder
  • Update dependencies in pwasm-utils

this is part of .2 and was doing outside of the gear repo

  • Refactor crates.io manager (can questionably be merged with the first one)

crates-io-manager is not in production for now since there are still some packages needed to be published but not included in its logic

@NikVolf
Copy link
Member

NikVolf commented Dec 1, 2023

@breathx ?

utils/crates-io/src/bin/publish.rs Outdated Show resolved Hide resolved
utils/crates-io/src/lib.rs Show resolved Hide resolved
utils/wasm-builder/Cargo.toml Outdated Show resolved Hide resolved
utils/crates-io/src/publisher.rs Outdated Show resolved Hide resolved
utils/crates-io/src/lib.rs Show resolved Hide resolved
utils/crates-io/src/publisher.rs Show resolved Hide resolved
gcli/Cargo.toml Outdated Show resolved Hide resolved
@clearloop clearloop merged commit 37fba1c into master Dec 5, 2023
11 checks passed
@clearloop clearloop deleted the cl/issue-3500 branch December 5, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview PR is ready to be reviewed by the team B1-releasenotes The feature deserves to be added to the Release Notes
Projects
None yet
6 participants