Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove substrate-in-the-browser #9541

Merged
13 commits merged into from
Aug 17, 2021
Merged

Remove substrate-in-the-browser #9541

13 commits merged into from
Aug 17, 2021

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Aug 11, 2021 •
edited
Loading

Polkadot companion: paritytech/polkadot#3652

There has been some discussion about the current status of the substrate-in-the-browser project and my understanding is that it is currently unused (and unloved?) and that the way forward lies in smoldot. This PR removes the code changes that were introduced as part of #2416.

Removing this code will help a bit when replacing jsonrpc by jsonrpsee and makes the telemetry code a bit easier to read.

@dvdplm dvdplm self-assigned this Aug 11, 2021
@dvdplm dvdplm added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-breaksapi labels Aug 11, 2021
@gilescope gilescope requested a review from tomaka August 11, 2021 13:13
@tomaka
Copy link
Contributor

tomaka commented Aug 11, 2021

Fine by me

@tomaka tomaka requested a review from expenses August 11, 2021 13:19
@expenses
Copy link
Contributor

This sounds like a good idea to me. o7 to the wasm node, you were a good idea at the time.

I'm unsure about simply commenting things out instead of removing them entirely, but if you want to take onboard the responsibility of removing them in the future then that's okay. Another thing to do would be to replace all instances of wasm_timer::Instant with std::time::Instant.

dvdplm
dvdplm commented Aug 11, 2021
View reviewed changes
.gitlab-ci.yml Outdated
@@ -391,6 +391,7 @@ test-linux-stable-int:
paths:
- ${CI_COMMIT_SHORT_SHA}_int_failure.log

# TODO: (dp) Not 100% sure what this does; maybe just rename the CI stage?
check-web-wasm:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TriplEight Do you have any input here? Ok if I just rename it to check-tracing or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIU it's a set of tests combined into one job so that reuse the cache better. FWIW now I'd rename it to check-wasm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thing is this PR removes compiling the substrate client to wasm (obviously the runtime remains in wasm) so that's not a great name. I'll go with check-tracing for now, let me know if you prefer something else!

@TriplEight
Copy link
Contributor

TriplEight commented Aug 11, 2021 •
edited
Loading

LGTM.
I was among those who asked this question.
My case was that I was migrating the CI image to ubuntu20.04 and due to reasons had to change the browser. This was a bit of a blocker, but have to say that in the end, I've made it work.
Thanks to this PR, I can remove a whole bunch of tools from the CI image now.

@gilescope
Copy link
Contributor

Better we remove rather than comment out. We have the git history.

@dvdplm
Copy link
Contributor Author

dvdplm commented Aug 15, 2021

Better we remove rather than comment out. We have the git history.

Yeah, this is a draft still. :)

I expected this to be tougher sell than this so wanted to limit the effort until I knew this was ok with people.

dvdplm
dvdplm commented Aug 15, 2021
View reviewed changes
client/telemetry/src/transport.rs Outdated
@@ -89,6 +74,7 @@ pub(crate) type WsTrans = libp2p::core::transport::Boxed<
///
/// For some context, we put this object around the `wasm_ext::ExtTransport` in order to make sure
/// that each telemetry message maps to one single call to `write` in the WASM FFI.
// TODO: (dp) Looks like we don't need this if we remove the `wasm_ext::ExtTransport`? Rework it without.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@expenses @tomaka I've looked a bit deeper at this code and I don't agree with my own comment any longer, it seems like StreamSink does what needs doing regardless of wasm_ext::ExtTransport. In other words: I'm inclined to leave this as-is (well, adjust the comment above ofc).

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, StreamSink is necessary anyway.

@dvdplm dvdplm added the D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. label Aug 16, 2021
@dvdplm dvdplm marked this pull request as ready for review August 16, 2021 19:41
@dvdplm dvdplm requested a review from a team as a code owner August 16, 2021 19:41
@dvdplm dvdplm mentioned this pull request Aug 17, 2021
Merged
TriplEight
TriplEight approved these changes Aug 17, 2021
View reviewed changes
Copy link
Contributor

@TriplEight TriplEight 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!

@dvdplm
Copy link
Contributor Author

dvdplm commented Aug 17, 2021

bot merge

@ghost
Copy link

ghost commented Aug 17, 2021

Trying merge.

@s3krit s3krit mentioned this pull request Aug 17, 2021
Merged
@s3krit
Copy link
Contributor

s3krit commented Aug 17, 2021 •
edited
Loading

I think we also want to remove check_web_wasm from polkadot once this change is merged. And also remove check_web_wasm from the required checks in polkadot + substrate in order to be able to merge. @TriplEight Would you be able to do that?

paritytech/polkadot#3654

@TriplEight
Copy link
Contributor

bot merge

@ghost
Copy link

ghost commented Aug 17, 2021

Trying merge.

@ghost ghost merged commit d2a43d4 into master Aug 17, 2021
@ghost ghost deleted the dp-remove-substrate-in-the-browser branch August 17, 2021 18:06
@chevdor chevdor mentioned this pull request Sep 7, 2021
Merged
@github-actions github-actions bot mentioned this pull request Oct 11, 2021
Closed
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TriplEight TriplEight TriplEight approved these changes

@expenses expenses expenses approved these changes

@bkchr bkchr bkchr approved these changes

@tomaka tomaka tomaka approved these changes

@andresilva andresilva Awaiting requested review from andresilva

Assignees

@dvdplm dvdplm

Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-breaksapi D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dvdplm @tomaka @expenses @TriplEight @gilescope @s3krit @bkchr