Revert "Skip binary for determinism checking" #49402
Conversation
|
VS_Integration release_32 failure appears to be due to a network issue (HTTP 429--is the CI machine getting rate limited?) |
|
Hello @AlekseyTs! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
ghost
added this to the 
|
It appears like this is failing again once we removed the skip here . https://dev.azure.com/dnceng/public/_build/results?buildId=888293 |
How do we know it is the same reason? |
I haven't done the diff yet so I'm not 100% sure but the timeline of the revert, the appearance of the failures and the assembly where the failures occur all line up. In the process of doing the diff at hte moment. |
|
Diff indicates this is the same issue. The case difference in Span is showing up again.
|
|
The casing of the field name now should match casing in the property name. Is it possible the fix isn't there for some reason, because I would expect to see the difference in the property name as well. |
|
Not sure where to write down how I investigate bugs like this so just going to throw it here for now. First step is to grab the determinism logs from the build
The before / after versions of the binaries will appear in the Left and Right sub-directory. From there I run the folloing in both directories
Then I just windiff the data.txt file and see what changed. There are a number of diffs here but most of them are just changing a token value, not the name. This is the only place where the name changes. |
Scrolling through the rest of the diff I see that's mor ethan just the field
The string table also shows the different casing (that diff doesn't really fit well into a screen shot but can provid if you want). |
|
So, there is a difference in parameter name, but not in property name? Or we don't check property names? |
|
I'll try following your steps to take a look at the binary itself |
The property name is the same here in both sets. The check we do here is a byte for byte comparison of the DLLs. So it checks literally everything that we output. The use of |
|
Clearly there was a problem with constructor parameters, but I cannot figure out how fields can be non-deterministic after the recent fix. Fixed parameters and added temporary throws to detect if our assumptions are violated in some obscure scenarios in #49467. Hopefully, instead of simply getting a comparison failure we'll get a call stack. |
|
@AlekseyTs This change has resulted in 8 failed rolling builds in the past 3 days. Can you revert this and file a follow-up issue? /cc @allisonchou |
|
@AlekseyTs thank you!! |
Reverts #48450