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

polkadot-parachain: compile separate lib and bin #5288

Merged
merged 18 commits into from
Aug 21, 2024

Conversation

serban300
Copy link
Contributor

@serban300 serban300 commented Aug 8, 2024

Related to #5210

@serban300 serban300 added the T0-node This PR/Issue is related to the topic “node”. label Aug 8, 2024
@serban300 serban300 self-assigned this Aug 8, 2024
@serban300 serban300 force-pushed the polkadot-parachain-lib branch 2 times, most recently from f77f319 to be75466 Compare August 8, 2024 16:14
@serban300 serban300 force-pushed the polkadot-parachain-lib branch from be75466 to f18d191 Compare August 8, 2024 16:59
@serban300 serban300 force-pushed the polkadot-parachain-lib branch from f18d191 to 650f007 Compare August 8, 2024 17:54
@serban300 serban300 marked this pull request as ready for review August 9, 2024 06:53
@serban300 serban300 requested a review from a team as a code owner August 9, 2024 06:53
@paritytech-review-bot paritytech-review-bot bot requested a review from a team August 9, 2024 06:54
@@ -14,13 +14,11 @@
// You should have received a copy of the GNU General Public License
// along with Cumulus. If not, see <http://www.gnu.org/licenses/>.

use crate::chain_spec::{
Copy link
Contributor

Choose a reason for hiding this comment

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

@athei is this parachain even needed anymore? perhaps we should remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are working on a new testnet for for pallet_revive, I think it's safe to get rid of this one now

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, something to do in a follow-up then @serban300 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened PR #5471 for this

umbrella/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

lgtm module some kind of usage doc. What can I do with this lib now? what benefit it has to a parachain team? (except if you are @xlc and you know because you requested this 😉)

You can put it in the main lib.rs, and in the PR description.


@PierreBesson from what I can see, you should brace for either:

  1. Parity needs to generate its own omni-node in order to keep things like --chain asset-hub backwards compatible.
  2. Parity should prepare to replace all --chain <fixed-string> to --chain <json-file>.

In all honesty, I regret not pushing harder for separating polkadot-parachain and omni-node. We could have left polkadot-parachain as is for eternity, and started working on a fresh fork of it, giving us all the flexibility in the world. The fact that the omni-node is called polkadot-parachain (we agreed to not rename it in a separate PR) also still sounds counterproductive to me. It is buzz-killer. I do hope we rename it someday once the dust has settled :)

The benefit of what we do now is it that we are forced to dogfood the process of going from --chain name to --chain spec.json.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: check-tracing
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6968216

[features]
default = []
runtime-benchmarks = [
"cumulus-primitives-core/runtime-benchmarks",
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably can remove all those features. I don't really know why they exists at the first place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's related to runtime-benchmarks I answered on a different comment below


[features]
default = []
runtime-benchmarks = [
Copy link
Contributor

Choose a reason for hiding this comment

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

what are those for? we should use the standalone bencher and try-runtime-cli

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need them in the CI for the bot ... commands. There is an item for removing them after we migrate the CI commands to the omni-bencher: #4966

@xlc
Copy link
Contributor

xlc commented Aug 13, 2024

I am trying this at https://github.com/AcalaNetwork/acala-node
The exposed API is easy enough.
The main issue I have is about deps. I shouldn't fine any runtime in Cargo.lock file. Ideally, it shouldn't contain any pallets and frame crates.
And maybe related to I am using a fork, many polkadot-sdk creates are pulled twice such as sp-std due to deps to creates at paritytech/arkworks-substrate

Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

you will need the build.rs file in the bin, not in the lib

@serban300
Copy link
Contributor Author

you will need the build.rs file in the bin, not in the lib

Done

@serban300
Copy link
Contributor Author

The main issue I have is about deps. I shouldn't fine any runtime in Cargo.lock file. Ideally, it shouldn't contain any pallets and frame crates.

Removed the rococo-native and westend-native. From what I see the pallets and frame crates are needed just for accessing some runtime APIs. We can move those runtime APIs definitions to primitives, but I would prefer to do it in a separate PR since this one is pretty verbose already.

And maybe related to I am using a fork, many polkadot-sdk creates are pulled twice such as sp-std due to deps to creates at paritytech/arkworks-substrate

I guess this is normal since probably both versions are needed.

Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Looks good and useful to me, nice work!

@serban300 serban300 added this pull request to the merge queue Aug 21, 2024
Merged via the queue into paritytech:master with commit 5ca3d2e Aug 21, 2024
188 of 189 checks passed
@serban300 serban300 deleted the polkadot-parachain-lib branch August 21, 2024 16:03
github-merge-queue bot pushed a commit that referenced this pull request Aug 28, 2024
Related to: #5210

Follow-up for #5288, as
per this comment:
#5288 (comment)

I hope I understood this correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants