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

Update to .NET 8 #5322

Merged
merged 28 commits into from
Jan 8, 2025
Merged

Update to .NET 8 #5322

merged 28 commits into from
Jan 8, 2025

Conversation

atomb
Copy link
Member

@atomb atomb commented Apr 15, 2024

Description

  • Use .NET 8.0 instead of 6.0 throughout the project
  • Make the ordering of diagnostics deterministic even when multiple diagnostics differ only in their related locations

How has this been tested?

  • Updated existing tests to match well-defined ordering

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@atomb atomb self-assigned this Apr 15, 2024
@atomb atomb mentioned this pull request Apr 23, 2024
@tchajed
Copy link
Contributor

tchajed commented Sep 16, 2024

I think it would be good to revisit this. Homebrew has deprecated the Dafny formula because it depends on dotnet 6 which is no longer supported upstream.

@keyboardDrummer
Copy link
Member

I think it would be good to revisit this. Homebrew has deprecated the Dafny formula because it depends on dotnet 6 which is no longer supported upstream.

Agreed. Would be good for Dafny development itself as well.

@keyboardDrummer
Copy link
Member

keyboardDrummer commented Nov 6, 2024

Fuel.legacy.dfy test becomes non-deterministic with this change. It flips between:

 Fuel.legacy.dfy(335,26): Error: function precondition could not be proved
 Fuel.legacy.dfy(324,21): Related location
-Fuel.legacy.dfy(313,41): Related location
+Fuel.legacy.dfy(314,46): Related location

Both error messages are valid, since they point to two different failing cases of a match.

.bpl files between conflicting runs are the same

Using /vcsCores:1 does not resolve the issue

@keyboardDrummer keyboardDrummer marked this pull request as ready for review January 7, 2025 13:28
@keyboardDrummer keyboardDrummer changed the title Try building with .NET 8.0 Update to .NET 8 Jan 7, 2025
@keyboardDrummer
Copy link
Member

Non-determinism issue has been resolved.

@keyboardDrummer keyboardDrummer enabled auto-merge (squash) January 7, 2025 13:29
@@ -37,7 +37,7 @@ jobs:
- name: Setup dotnet
uses: actions/setup-dotnet@v4
with:
dotnet-version: 6.0.x
dotnet-version: 8.0.x
Copy link
Member

Choose a reason for hiding this comment

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

Okay now we really are installing the same version of .NET twice in several workflows, and my comments from an earlier PR to remove the duplication actually apply now :)

@@ -2,7 +2,7 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net6.0</TargetFramework>
<TargetFramework>net8.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

Could this be moved to Directory.Build.props so it only appears once?

var leftSideDots = leftSide.OfType<ExprDotName>();
var rightSideDots = rightSide.OfType<ExprDotName>();
var allExprDotNames = leftSideDots.Concat(rightSideDots);
var allExprDotNames = leftSide.OfType<ExprDotName>();
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Member

Choose a reason for hiding this comment

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

@atomb made this change. Rider confirms it's a noop:

image

Same for the other one I imagine.

Copy link
Member

Choose a reason for hiding this comment

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

Ah got it, definitely not obvious without context

Copy link
Member

Choose a reason for hiding this comment

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

How did Fuel.legacy.dfy.expect ever work before?

Copy link
Member

@keyboardDrummer keyboardDrummer Jan 7, 2025

Choose a reason for hiding this comment

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

I think it relied on the undefined but deterministic ordering of something, like enumerating over a dictionary. I'm guessing there was a performance improvement in .NET that made this undefined ordering also non-deterministic, which caused the test to become unstable until I added an explicit ordering.

@@ -67,7 +67,7 @@ protected override void EmitHeader(Program program, ConcreteSyntaxTree wr) {
wr.WriteLine("// Dafny program {0} compiled into C#", program.Name);
wr.WriteLine("// To recompile, you will need the libraries");
wr.WriteLine("// System.Runtime.Numerics.dll System.Collections.Immutable.dll");
wr.WriteLine("// but the 'dotnet' tool in net6.0 should pick those up automatically.");
wr.WriteLine("// but the 'dotnet' tool in net8.0 should pick those up automatically.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
wr.WriteLine("// but the 'dotnet' tool in net8.0 should pick those up automatically.");
wr.WriteLine("// but the 'dotnet' tool in .NET should pick those up automatically.");

Just to avoid some diffs the next time we upgrade.

@keyboardDrummer keyboardDrummer enabled auto-merge (squash) January 8, 2025 10:03
Copy link
Member

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

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

Excelsior!

@keyboardDrummer keyboardDrummer merged commit 0831717 into dafny-lang:master Jan 8, 2025
22 checks passed
olivier-aws pushed a commit to olivier-aws/dafny that referenced this pull request Jan 13, 2025
### Description
- Use .NET 8.0 instead of 6.0 throughout the project
- Make the ordering of diagnostics deterministic even when multiple
diagnostics differ only in their related locations

### How has this been tested?
- Updated existing tests to match well-defined ordering

<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>

---------

Co-authored-by: Remy Willems <[email protected]>
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