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

Revive Substrate Crate #1477

Merged
merged 8 commits into from Oct 6, 2023
Merged

Revive Substrate Crate #1477

merged 8 commits into from Oct 6, 2023

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Sep 8, 2023

Closes #1450

Bringing back the Substrate crate that was forgotten in the monorepo import 😅.
It is a doc-only crate. Version number is set to 1.0.0 and publishing is enabled (so that we can link to docs.rs).

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

IMO we should move the crate into its own folder. It shouldn't at the top of the substrate folder.

@ggwpez
Copy link
Member Author

ggwpez commented Sep 9, 2023

IMO we should move the crate into its own folder. It shouldn't at the top of the substrate folder.

Where else should it go? This crate is documenting the whole Substrate Framework.
I can put it into a doc folder if you want though.

@gilescope
Copy link
Contributor

Then cargo check in the substrate dir would just check substrate code. It's good to have options as to how much code you want to compile. (E.g. you might want to get something working happily with the substrate codebase before then dealing with the impacts to the cumulus and polkadot dirs.) Compiling the kitchen sink would get you this but that's not as intuitive.

substrate/Cargo.toml Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Sep 9, 2023

I can put it into a doc folder if you want though.

Good idea 👍

@bkchr
Copy link
Member

bkchr commented Sep 9, 2023

It's good to have options as to how much code you want to compile

This option is the -p parameter to compile exactly what you need. You can also write yourself a script if you really need to have everything compiled in the substrate folder, which is generally just a bad idea. Test it crate locally and then you can check everything.

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.

truthfully I prefer root of the folder to 'doc' as it is more aligned with our naming conventions. Otherwise we'd have to make a crate in 'substrate/doc' named 'substrate', which is a new convention now.

Either way good though

@bkchr
Copy link
Member

bkchr commented Sep 10, 2023

I would have thought we create a top level folder doc? We probably want the same doc process for everything.

Co-authored-by: Bastian Köcher <git@kchr.de>
substrate/Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member Author

ggwpez commented Sep 12, 2023

I would have thought we create a top level folder doc? We probably want the same doc process for everything.

You mean since otherwise it would be inconsistent with the root level? I guess it would looks like this:

  • /doc/Cargo.toml polkadot-sdk doc crate
  • /substrate/Cargo.toml ...
  • /polkadot/Cargo.toml ...
  • /cumulus/Cargo.toml ...

Yea it would be slightly inconsistent, but that it only since the root level should not have a src folder. I think for the other ones it is fine, or?

@bkchr
Copy link
Member

bkchr commented Sep 12, 2023

I mean in general, if we have doc only crates. Which is fine. Why should the not exist in doc/substrate or whatever. I'm also not that much interested where we put it, it just "feels" weird to have there a crate in the substrate folder.

@wentelteefje
Copy link
Contributor

wentelteefje commented Sep 12, 2023

I mean in general, if we have doc only crates. Which is fine. Why should the not exist in doc/substrate or whatever. I'm also not that much interested where we put it, it just "feels" weird to have there a crate in the substrate folder.

My idea is to have one top-level crate, which links to the doc-only crates for substrate, polkadot and cumulus (almost like the README of the git repo itself). I think it doesn't really matter where exactly these four crates live in the repository, but I'm not really happy with introducing a doc folder at the root level, because then we have both a doc and a docs (which has some MD-files for contributions etc) folder there. Maybe we can just create four crate folders in docs/ like so:

docs/top-level-doc
docs/substrate
docs/polkadot
docs/substrate

Would that cause any other issues? The user journey I've had in my looks something like this:

paritytech.github.io/polkadot-sdk -> top-level crate -> User can select between doc-only crates Substrate/Polkadot/Cumulus as their entry point to the projects

@bkchr
Copy link
Member

bkchr commented Sep 13, 2023

Wasn't aware that we already have doc 🙈 Let's use docs.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Cargo.toml Outdated
@@ -96,6 +96,7 @@ members = [
"cumulus/test/runtime",
"cumulus/test/service",
"cumulus/xcm/xcm-emulator",
"docs/substrate",
Copy link
Contributor

Choose a reason for hiding this comment

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

@wentelteefje can you make sure that this crate is opened when you open substrate in https://paritytech.github.io/polkadot-sdk/? I think it was some line that has to be changed in the CI configs.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@@ -0,0 +1,28 @@
[package]
Copy link
Contributor

Choose a reason for hiding this comment

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

should be removed.

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.

I am mildly against the folder structure here and want to bikeshed in favor of moving this back to ./substrate/src in its current structure.

I just think it is inconsistent with the previous pattern. Also, when we want to create automatic redirects for dead URLs like https://paritytech.github.io/substrate/master/substrate/index.html, we would have to make an exception for this.

Also the naming of what goes in ./doc and what goes in ./docs is not clear.

But happy to move forward as is.

Now question for @bkchr: if I am to create a new crate that is analogous to the substrate crate for polkadot-sdk, polkadot and cumulus, all of thse would also be placed under ./docs?

@kianenigma kianenigma requested a review from a team September 19, 2023 15:43
@sam0x17
Copy link
Contributor

sam0x17 commented Sep 20, 2023

Yeah, I've said this before but principle of least surprise in a rust workspace, at least for me, would dictate that any directory that has crates in it should itself be a crate containing re-exports of all the child crates (except for private/internal ones). This allows for granular compiling based on the folder structure and I would consider it a best practice.

If this is for some reason awkward to follow then this is a smell that something is wonky with our directory structure. The directory hierarchy and our crate/module hierarchy should work together in close unison to organize our code as much as possible.

Even when things aren't set up this way, many times people will cd into the substrate directory, run a cargo command, and have the entirely reasonable expectation that the cargo command should build substrate, not polkadot-sdk. It is weird/bad that we don't follow this everywhere, some examples being the pallets directory, the frame directory, and I'm sure others that I'm not familiar with.

There are numerous benefits to this setup beyond mere docs accessibility, and in lieu of additional bin targets, this doesn't increase build time at all.

@kianenigma
Copy link
Contributor

Yeah, I've said this before but principle of least surprise in a rust workspace, at least for me, would dictate that any directory that has crates in it should itself be a crate containing re-exports of all the child crates (except for private/internal ones). This allows for granular compiling based on the folder structure and I would consider it a best practice.

Yeah, and once we move ./pallets into a different top level folder (as discussed in Malmo) this ideal that you are posing would be possible:

./frame
   ./src
./pallets
   ./src 
   ./sudo
   ./alliance
   ./...

Then I would be happy with having a crate that is just all pallets bundled together. It would be a rather stupid crate to use as you rarely need all pallets, but it would be harmless as it won't pollute the frame pallet.

@kianenigma
Copy link
Contributor

Now question for @bkchr: if I am to create a new crate that is analogous to the substrate crate for polkadot-sdk, polkadot and cumulus, all of thse would also be placed under ./docs?

Also, we want to have a slim version of the master tutorial and reference docs to live in the mono-repo for compatibility and versioning reasons. Where should those live.

The more I think about this, the more I lean toward:

  1. rename what is called now docs to documents to signify this is about non-core files (aka documents), and avoid confusing with it being interpreted as documentation.
  2. Move prdoc related stuff in there as well cc @chevdor
  3. All umbrella crates such as polkadot-sdk, substrate, polkadot and cumulus should live where they logically belong in file tree.
  4. The main tutorial and reference docs should reside under a new folder called devhub. So, something like:
devhub/
├─ tutorial
│  ├─ step1.rs
│  ├─ step2.rs
│  ├─ see https://github.com/paritytech/polkadot-sdk-docs/issues/3
├─ reference-docs/
│  ├─ extrinsics.rs
│  ├─ account_abstraction.rs
│  ├─ see https://github.com/paritytech/polkadot-sdk-docs/issues/4
documents/
│  ├─ foo.md
│  ├─ bar.adoc
polkadot/
substrate/
cumulus/

@bkchr
Copy link
Member

bkchr commented Oct 5, 2023

All umbrella crates such as polkadot-sdk, substrate, polkadot and cumulus should live where they logically belong in file tree.

If there are only exist for doc reasons, there logical place should be in the docs folder 👀

But yeah, do whatever you want to do. I will not block on this.

@ggwpez ggwpez added the T11-documentation This PR/Issue is related to documentation. label Oct 5, 2023
@ggwpez
Copy link
Member Author

ggwpez commented Oct 5, 2023

I moved it back to substrate/Cargo.toml now. Then lets go once the CI is green!

@kianenigma kianenigma merged commit 1835c09 into master Oct 6, 2023
109 checks passed
@kianenigma kianenigma deleted the oty-revive-substrate branch October 6, 2023 08:08
@sacha-l sacha-l mentioned this pull request Oct 10, 2023
@kianenigma
Copy link
Contributor

moved it back to substrate/Cargo.toml now. Then lets go once the CI is green

I am sorry to say that my proposal above was not great and I am about to change my mind 🙈 I think we should move everything into a single crate called devhub and let all written documentation be mods in this crate.

This gives us the ability to navigate backwards and inwards into different crates, and build a nice hierarchy. For example, you could be in devhub::polkadot_sdk::substrate::frame and the fact that you can see this path at the top of the page and go backwards is a big win. We cannot really have this nice experience if we have independent crates.

I think I will explore further with this and possibly change the structure. Once this structure is settled we should finalize the future of markdown files as well #1260.

bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Closes paritytech#1450

Bringing back the Substrate crate that was forgotten in the monorepo
import 😅.
It is a doc-only crate. Version number is set to `1.0.0` and publishing
is enabled (so that we can link to docs.rs).

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T11-documentation This PR/Issue is related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring back Substrate entry point crate
8 participants