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 C# module smoketests #1475

Merged
merged 25 commits into from
Jul 4, 2024
Merged

Fix C# module smoketests #1475

merged 25 commits into from
Jul 4, 2024

Conversation

bfops
Copy link
Collaborator

@bfops bfops commented Jul 2, 2024

Description of Changes

#1440 restructured our NuGet packages, and correspondingly updated the .csproj files we generate. However, since we haven't actually published the restructured NuGet packages, .csprojs expecting the new NuGet package structure will be broken, including the smoketests.

It seems like this behavior was sometimes causing smoketests to fail. It's unclear why it's only sometimes, but our best guess is "something something caching", so this PR forcibly clears the NuGet package cache.

Additionally, we bump the versions of the local NuGet packages to 0.11, so that they cannot be found on the NuGet servers. This means that the smoketests can only find them in local directories, so they do not get confused about where to get these packages.

API and ABI breaking changes

No

Testing

@bfops bfops marked this pull request as ready for review July 2, 2024 19:22
Shubham8287
Shubham8287 previously approved these changes Jul 2, 2024
Copy link
Contributor

@Shubham8287 Shubham8287 left a comment

Choose a reason for hiding this comment

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

Gets the job done!

@Shubham8287
Copy link
Contributor

Oh wait, The PR in description fails with this error -

/tmp/tmp6wbnwk4t/Lib.cs(8,29): error CS0103: The name 'ColumnAttrs' does not exist in the current context [/tmp/tmp6wbnwk4t/StdbModule.csproj]

Is that means, error is specific to those PRs?

@bfops bfops marked this pull request as draft July 2, 2024 21:40
@bfops bfops dismissed Shubham8287’s stale review July 3, 2024 20:23

branch has changed meaningfully

@bfops bfops changed the title Fix spacetime generate creating broken C# projects Fix C# module smoketests Jul 3, 2024
@@ -17,6 +17,7 @@ def test_build_csharp_module(self):

try:

run_cmd("dotnet", "nuget", "locals", "all", "--clear", cwd=bindings, capture_stderr=True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

clear package cache. this isn't required to make the CI work again, but there did seem to be some kind of caching happening because the smoketests weren't passing or failing consistently. This hopefully makes them more consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(we think this also resolves a hypothetical issue where multiple open PRs make non-backwards-compatible changes to the NuGet packages)

@bfops bfops marked this pull request as ready for review July 3, 2024 22:53
@bfops bfops enabled auto-merge July 3, 2024 22:59
Copy link
Contributor

@Shubham8287 Shubham8287 left a comment

Choose a reason for hiding this comment

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

This feels like a good mitigation to fix the master. Thanks for working on this.
But I think there is still a need to have a general solution so that we don't run into the problem in future (not to be done by you or under the scope of this PR).

@bfops bfops added this pull request to the merge queue Jul 4, 2024
Merged via the queue into master with commit b2eb08c Jul 4, 2024
10 checks passed
@bfops bfops deleted the bfops/fix-csharp-codegen branch July 8, 2024 16:34
@bfops
Copy link
Collaborator Author

bfops commented Jul 8, 2024

This feels like a good mitigation to fix the master. Thanks for working on this. But I think there is still a need to have a general solution so that we don't run into the problem in future (not to be done by you or under the scope of this PR).

The general solution is to bump the package versions when there are breaking changes

lcodes pushed a commit that referenced this pull request Jul 9, 2024
Co-authored-by: Zeke Foppa <github.com/bfops>
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