Skip to content

Conversation

marcocastignoli
Copy link
Member

For some reason Vyper outputs warnings in stderr. With this PR we are handling this case.

@kuzdogan
Copy link
Member

kuzdogan commented Aug 11, 2025

Maybe it's better we handle it all in vyperCompiler.ts as we do in the SolidityCompiler?

https://github.com/ethereum/sourcify/blob/0a417e6de521ee23a8e3d19b9192780d36c2ba7e/packages/compilers/src/lib/vyperCompiler.ts#L80-L89

I'm asking because I'm wondering if we throw when we see something in the stdErr, are we going to see these as structured and meaningful errors in a verificationJob? Or just an unstructured string that is coming from the stdErr?

From what I can see it won't because you are checking startsWith which suggests this is a simple string.

Also what happens if there's a Warning in the beginning and errors come later?

@kuzdogan kuzdogan self-assigned this Aug 11, 2025
@marcocastignoli
Copy link
Member Author

just an unstructured string that is coming from the stdErr

The one I see was just an unstructured string, so I don't think an advanced parsing is needed.

@kuzdogan
Copy link
Member

Ok but to my point, when we throw it on the stdErr output, we don't get the structured errors like this one https://verify.sourcify.dev/jobs/65f64b95-2e1b-4a8f-9db3-66985b1c15bd

and also this: "Also what happens if there's a Warning in the beginning and errors come later?"

@marcocastignoli
Copy link
Member Author

Also what happens if there's a Warning in the beginning and errors come later?

Ah! I lost this one, it actually makes sense. Then unfortunately we need to dig deeper into this and create some tests to explore all the cases :(

@kuzdogan
Copy link
Member

How about ignoring the stderr and looking for compiler errors in the std JSON output? I wonder if this line of checking stdErr does ever get triggered in Solidity because this asyncExec is a shared function. We need to check how the changes affect both solidity and vyper compilers

@marcocastignoli
Copy link
Member Author

I'll move forward with this by creating some tests with different errors / warnings for both solidity and vyper. Then we'll adapt the fix so that it doesn't break any test.

@marcocastignoli marcocastignoli marked this pull request as draft August 14, 2025 07:04
@manuelwedler manuelwedler moved this from Triage to Sprint - Up Next in Sourcify Public [Archived] Aug 19, 2025
@marcocastignoli
Copy link
Member Author

I discovered that Vyper < 0.4.0 outputs warnings in the stderr, while vyper >=0.4.0 includes them correctly in the errors object.

@kuzdogan I think the approach of this PR is correct, I added the tests for Vyper to make sure we handle cases (< 0.4.0 AND >= 0.4.0), what do you think?

@manuelwedler manuelwedler marked this pull request as ready for review August 21, 2025 07:41
@marcocastignoli marcocastignoli merged commit 3eba136 into staging Aug 21, 2025
3 of 4 checks passed
@github-project-automation github-project-automation bot moved this from Sprint - Up Next to Sprint - Done in Sourcify Public [Archived] Aug 21, 2025
@marcocastignoli marcocastignoli deleted the handle-vyper-warnings branch August 21, 2025 09:25
@stackenbotten3000 stackenbotten3000 moved this to Sprint - Done in Sourcify Public Aug 27, 2025
@manuelwedler manuelwedler moved this from Sprint - Done to COMPLETED in Sourcify Public Aug 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

3 participants