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

Export remaining public functions of sc-service #18

Merged
merged 1 commit into from
May 16, 2024

Conversation

dastansam
Copy link

@dastansam dastansam commented May 14, 2024

while working on this issue autonomys/subspace#2765, I found out that functions used in spawn_tasks() were converted to public but was not added to exported types/functions of sc-service

@dastansam dastansam changed the title Export remaining public functions of sc-service builder Export remaining public functions of sc-service May 14, 2024
@dastansam dastansam requested a review from nazar-pc May 14, 2024 15:32
Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Hm... I really hate re-exports 😕

Could you send this upstream too, please?

@dastansam
Copy link
Author

Hm... I really hate re-exports 😕

Could you send this upstream too, please?

yes, in progress

@nazar-pc
Copy link
Member

I generally open PR upstream first, then depending on how quickly they review it backport into Subspace fork. This way we can ensure in most cases that we land the same code that upstream does and not have merge conflicts down the line. Especially with PRs that are this simple.

@dastansam
Copy link
Author

I generally open PR upstream first, then depending on how quickly they review it backport into Subspace fork. This way we can ensure in most cases that we land the same code that upstream does and not have merge conflicts down the line. Especially with PRs that are this simple.

upstream PR was merged yesterday, how do you usually backport it? do we have to release a new branch subspace-v6?

@nazar-pc
Copy link
Member

No, if upstream PR is exactly the same as this branch (I see no link to it) then we're okay to merge this alrady. Otherwise I cherry-pick commits from upstream the way they were merged there.

@dastansam
Copy link
Author

dastansam commented May 16, 2024

No, if upstream PR is exactly the same as this branch (I see no link to it) then we're okay to merge this alrady. Otherwise I cherry-pick commits from upstream the way they were merged there.

ah sorry paritytech#4457, the only difference is the prdoс

@nazar-pc
Copy link
Member

ah sorry paritytech#4457, the only difference is the prdoс

It is fine, it'll not cause merge conflicts. We're good to go here 👍

@dastansam dastansam merged commit 9b8cdb8 into subspace-v5 May 16, 2024
9 of 13 checks passed
@dastansam
Copy link
Author

ah sorry paritytech#4457, the only difference is the prdoс

It is fine, it'll not cause merge conflicts. We're good to go here 👍

I will go ahead and open PRs to update commit hashes of polkadot-sdk deps

@nazar-pc nazar-pc deleted the export-sc-service-functions branch May 16, 2024 13:39
@nazar-pc
Copy link
Member

You'll have to update hash of the polkadot-sdk in our fork of Frontier as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants