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

Fix parsing of docker STDOUT #1526

Merged
merged 4 commits into from
Mar 7, 2024
Merged

Fix parsing of docker STDOUT #1526

merged 4 commits into from
Mar 7, 2024

Conversation

SkymanOne
Copy link
Contributor

@SkymanOne SkymanOne commented Mar 7, 2024

Summary

Closes #1455

  • y/n | Does it introduce breaking changes?
  • y/n | Is it dependent on the specific version of ink or pallet-contracts?

It was previously common to see the following error at the end of a verifiable build, even when the build was successful:
image

The previous implementation expected the BuildResult to be sent in a stream as a whole chunk of bytes. However, there can be cases when the data is sent in multiple chunks, which results in incorrect parsing.

We simply collect the byte chunks and then attempt to decode them together.

Checklist before requesting a review

  • My code follows the style guidelines of this project
  • I have added an entry to CHANGELOG.md
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

crates/build/src/docker.rs Outdated Show resolved Hide resolved
@smiasojed
Copy link
Collaborator

I think it would be great to have a CI test, which could catch issues like this.

@SkymanOne
Copy link
Contributor Author

I think it would be great to have a CI test, which could catch issues like this.

The problem is that we need to have a nested docker environment. Idk if it's possible to run docker container inside another container.

@smiasojed
Copy link
Collaborator

I think it would be great to have a CI test, which could catch issues like this.

The problem is that we need to have a nested docker environment. Idk if it's possible to run docker container inside another container.

I do not see any docker image used for CI, it seems that we are building this directly on the runner

@SkymanOne
Copy link
Contributor Author

I think it would be great to have a CI test, which could catch issues like this.

The problem is that we need to have a nested docker environment. Idk if it's possible to run docker container inside another container.

I do not see any docker image used for CI, it seems that we are building this directly on the runner

Sure, I'll do it in the follow up PR then

@cmichi
Copy link
Collaborator

cmichi commented Mar 7, 2024

Sure, I'll do it in the follow up PR then

@SkymanOne Please create a GitHub issue for it and assign it to yourself.

@cmichi cmichi merged commit 3b468df into master Mar 7, 2024
11 checks passed
@cmichi cmichi deleted the gn/fix-docker-stdout branch March 7, 2024 12:43
This was referenced Mar 8, 2024
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.

Unable to build a verifiable ink! flipper contract
4 participants