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

Tolerate plugins with warnings #437

Merged
merged 2 commits into from
Jan 27, 2025
Merged

Tolerate plugins with warnings #437

merged 2 commits into from
Jan 27, 2025

Conversation

Frassle
Copy link
Member

@Frassle Frassle commented Dec 20, 2024

Found this while on my adventures with dotnet plugins. If your plugin project has any warnings it's unusable in shimless mode (i.e. RunPlugin) because msbuild would first print the warning then start the plugin and print the port number. This lead to errors like:

error:  plugin [E:\VisualStudio\Projects\pulumi\jsonschema\Provider\pulumi-resource-Provider] wrote an invalid port to stdout: could not parse port: strconv.Atoi: parsing "E:\\VisualStudio\\Projects\\pulumi\\jsonschema\\Provider\\Provider.fsproj : warning NU1903: Package 'System.Text.Json' 6.0.2 has a known high severity vulnerability, https://github.com/advisories/GHSA-8g4q-xg66-9fp4": invalid syntax

When running shimless these warnings don't really matter. So this PR rewrites RunPlugin to do the build separately, and then invoke dotnet run with the --no-build flag. I would have preferred to just use dotnet run but there doesn't seem to be anyway to pass it flags to silence the msbuild output from it.

This is tested by adding a warning to the testprovider project that always triggers. As we use the testprovider project in shimless mode in the integration tests this shows that a project with warnings can still be used.

@Frassle Frassle marked this pull request as ready for review December 20, 2024 22:27
@Frassle Frassle requested a review from a team as a code owner December 20, 2024 22:27
Copy link
Contributor

@tgummerer tgummerer left a comment

Choose a reason for hiding this comment

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

Thanks, this seems like a good idea to me!

@Frassle
Copy link
Member Author

Frassle commented Jan 27, 2025

Seems something is off with the tests not using the changes in the language host, works fine locally.

@julienp
Copy link
Contributor

julienp commented Jan 27, 2025

Seems something is off with the tests not using the changes in the language host, works fine locally.

Looking at CI, I don't think we're building the language runtime in the integration tests 😬 I thought I noticed that earlier and fixed it, but maybe that was for java?

@julienp
Copy link
Contributor

julienp commented Jan 27, 2025

Ah no, we build in the makefile for test_integration, but I guess the built binary is not in the path?

@Frassle
Copy link
Member Author

Frassle commented Jan 27, 2025

but I guess the built binary is not in the path?

No it's not. Most of the integration tests change the PATH env to pick up the right thing but in the places we do NewEnvironment we're not getting that edit.

@Frassle Frassle added this pull request to the merge queue Jan 27, 2025
Merged via the queue into main with commit 9e965b9 Jan 27, 2025
19 checks passed
@Frassle Frassle deleted the fraser/runPlugin branch January 27, 2025 17:40
@pulumi-bot
Copy link

This PR has been shipped in release v3.72.0.

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.

4 participants