Skip to content

Reverts https://github.com/dotnet/corefx/pull/37583 ("Use clonefile for CopyFile, if available #37583")#40753

Merged
jozkee merged 4 commits intodotnet:masterfrom
jozkee:revert_corefx37583
Aug 15, 2020
Merged

Reverts https://github.com/dotnet/corefx/pull/37583 ("Use clonefile for CopyFile, if available #37583")#40753
jozkee merged 4 commits intodotnet:masterfrom
jozkee:revert_corefx37583

Conversation

@jozkee
Copy link
Member

@jozkee jozkee commented Aug 13, 2020

Fixes #31429
We will bring back the reverted change on 6.0 and ensure that doesn't regress functionality

@jozkee jozkee added this to the 5.0.0 milestone Aug 13, 2020
@jozkee jozkee requested a review from carlossanlop August 13, 2020 06:11
@jozkee jozkee self-assigned this Aug 13, 2020
@carlossanlop
Copy link
Contributor

carlossanlop commented Aug 13, 2020

cc @filipnavara

carlossanlop
carlossanlop reviewed Aug 13, 2020
View reviewed changes
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Thanks for making the change, @jozkee. It looks good, let's wait for the CI results.

@carlossanlop
Copy link
Contributor

carlossanlop commented Aug 13, 2020

The WASM CI leg failed with errors that seem related to this change:


[20:10:22] dbug: test[0]
      [FAIL] System.IO.Tests.FileInfo_CopyTo_str_b.CopyFileWithData(data: [], readOnly: False)
[20:10:22] dbug: test[0]
      [FAIL] System.IO.Tests.FileInfo_CopyTo_str_b.CopyFileWithData(data: [0x008f], readOnly: False)
[20:10:23] dbug: test[0]
      [FAIL] System.IO.Tests.FileInfo_CopyTo_str_b.CopyFileWithData(data: ['D', '¢', 'Y', 0x009c, 'é', ...], readOnly: False)
[20:10:23] dbug: test[0]
      [FAIL] System.IO.Tests.FileInfo_CopyTo_str_b.CopyFileWithData(data: ['ö', 0x0080, '\r', '×', 'Ö', ...], readOnly: False)
[20:10:27] dbug: test[0]
      [FAIL] System.IO.Tests.FileInfo_CopyTo_str_b.CopyFileWithData(data: ['3', 'F', 'ÃŽ', 0x008e, 0x000e, ...], readOnly: False)
...
...

@safern
Copy link
Member

safern commented Aug 13, 2020 •
edited by jozkee
Loading

@steveisok this seems related to the tests we just enabled? Could you help @jozkee here? Should we open an issue and disable the tests and then in 6.0 enable them once we have this fix in? cc: @lewing

@danmoseley danmoseley changed the title Reverts https://github.com/dotnet/corefx/pull/37583 Reverts https://github.com/dotnet/corefx/pull/37583 ("Use clonefile for CopyFile, if available #37583") Aug 14, 2020
@steveisok
Copy link
Member

steveisok commented Aug 14, 2020

Hmm - I think something / somethings in copy may be broken with this change. Unfortunately, we need to look closer to be able to tell.

@steveisok
Copy link
Member

steveisok commented Aug 14, 2020

Actually, the failures are Assert.InRange() Failure\r\nRange: (08/13/2020 08:08:37 - 08/13/2020 08:08:39)\r\nActual: 08/13/2020 07:08:38. Is this branch behind master to some degree?

@jozkee
Copy link
Member Author

jozkee commented Aug 14, 2020

@steveisok I just merged with master but seems that the Assert.InRange failure persists.

A new test added on this PR also fails for wasm:

[22:39:38] dbug: test[0]
      [FAIL] System.IO.Tests.File_Copy_Single.EnsureThrowWhenCopyToNonSharedFile
      
[22:39:38] dbug: test[0]
      Assert.Throws() Failure
      
[22:39:38] dbug: test[0]
      Expected: typeof(System.IO.IOException)
      
[22:39:38] dbug: test[0]
      Actual:   (No exception was thrown)

Is this something that we could defer to RC2?
@danmosemsft @jeffhandley

@steveisok
Copy link
Member

steveisok commented Aug 14, 2020

I say disable the tests and we can take a closer look.

@jozkee jozkee mentioned this pull request Aug 14, 2020
Closed
@jozkee
Copy link
Member Author

jozkee commented Aug 15, 2020

Current CI issues are unrelated:
#40881
#11063

@jozkee jozkee merged commit c00478b into dotnet:master Aug 15, 2020
@mdh1418 mdh1418 mentioned this pull request Aug 18, 2020
Merged
@MichaIng MichaIng mentioned this pull request Sep 12, 2020
Open
@MichaIng MichaIng mentioned this pull request Oct 21, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@jozkee jozkee deleted the revert_corefx37583 branch March 24, 2021 19:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@carlossanlop carlossanlop carlossanlop approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jozkee jozkee

Labels

area-System.IO

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

File.Copy no longer respects FileShare.None on Linux/Mac in 5.0

4 participants

@jozkee @carlossanlop @safern @steveisok