Skip to content

File copying on Windows buildbots is broken #56125

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

Closed
aam opened this issue Jul 2, 2024 · 10 comments
Closed

File copying on Windows buildbots is broken #56125

aam opened this issue Jul 2, 2024 · 10 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. gardening os-windows triaged Issue has been triaged by sub team

Comments

@aam
Copy link
Contributor

aam commented Jul 2, 2024

For example https://ci.chromium.org/ui/p/dart/builders/ci.sandbox/vm-aot-win-debug-x64/167/overview

Failure:

INFO: Enabling coredump archiving into C:\b\s\w\ir\cache\builder\sdk\crashes
INFO: Core dump archiving is activated
No build targets found.
Test configuration:
    vm-aot-win-debug-x64(architecture: x64, compiler: dartkp, mode: debug, runtime: dart_precompiled, system: win)
Suites tested: co19, corelib, ffi, kernel, language, lib, samples, standalone, utils, vm
Unhandled exception:
FileSystemException: Cannot copy file to 'C:/b/s/w/ir/cache/builder/sdk/out/DebugX64/generated_compilations/vm-aot-win-debug-x64/runtime_tests_vm_dart_split_aot_kernel_generation2_test/../../../../tests/language/mixin/regress_flutter_55345_test.dart', path = 'C:\b\s\w\ir\cache\builder\sdk\tests\language\mixin\regress_flutter_55345_test.dart' (OS Error: The filename, directory name, or volume label syntax is incorrect.
, errno = 123)
#0      _File.throwIfError (dart:io/file_impl.dart:675:7)
#1      _File.copySync (dart:io/file_impl.dart:366:5)
#2      StandardTestSuite._makeCommands (package:test_runner/src/test_suite.dart:850:39)
#3      StandardTestSuite._enqueueStandardTest (package:test_runner/src/test_suite.dart:770:22)
#4      StandardTestSuite._testCasesFromTestFile (package:test_runner/src/test_suite.dart:751:7)
#5      StandardTestSuite.findTestCases (package:test_runner/src/test_suite.dart:660:7)
#6      TestCaseEnqueuer.enqueueTestSuites (package:test_runner/src/process_queue.dart:234:13)
#7      new ProcessQueue (package:test_runner/src/process_queue.dart:172:22)
#8      testConfigurations (package:test_runner/src/test_configurations.dart:283:3)
#9      main (file:///C:/b/s/w/ir/cache/builder/sdk/pkg/test_runner/bin/test_runner.dart:52:9)
<asynchronous suspension>
INFO: No unexpected crashes recorded
***************** Killing dart *****************
  No dart processes found.
***************** Killing gen_snapshot *****************
  No gen_snapshot processes found.
***************** Killing dartaotruntime *****************
  No dartaotruntime processes found.
***************** Killing dart_precompiled_runtime *****************
  No dart_precompiled_runtime processes found.

cc @mraleph

@aam aam added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. os-windows gardening labels Jul 2, 2024
@aam
Copy link
Contributor Author

aam commented Jul 2, 2024

The culprit is https://dart-review.git.corp.google.com/c/sdk/+/372400 which started to convert destination to long-prefixed form.

@aam
Copy link
Contributor Author

aam commented Jul 2, 2024

https://dart-review.googlesource.com/c/sdk/+/374220 with proposed fix

copybara-service bot pushed a commit that referenced this issue Jul 3, 2024
Long-prefixed paths can't have '..' segments in them.

Delete recursively uses long-prefixes, so the tests that verify long paths handling had to be updated accordingly: it's okay to use long paths when they are prefixed(when they are deleted recursively).

This is follow-up to 4928998

Bug: #56125
Change-Id: I6613b1a324e8923d05e5f0a470ee5b2999d1b929
TEST=ci
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/374220
Commit-Queue: Alexander Aprelev <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>
@aam
Copy link
Contributor Author

aam commented Jul 3, 2024

and https://dart-review.googlesource.com/c/sdk/+/374181 with the prebuilt roll

@mraleph
Copy link
Member

mraleph commented Jul 3, 2024

Thanks for looking at this @aam.

Note that the fix landed in b207aed is incomplete I think. What this bug shows is that our IsAbsolutePath predicate is incorrect to begin with. So the fix did not fix the full problem e.g. consider situations where force_long_prefix is false but we have long "pseudo-absolute" path which starts with C:\ but has .. segments inside.

I wonder if we should just start using PathCchCanonicalizeEx? Our minimum supported Windows version allows us to use it.

@aam
Copy link
Contributor Author

aam commented Jul 3, 2024

Good point! But my thinking was that it's only when we use force_long_prefix the .. segments cause issues in "absolute paths".

@mraleph
Copy link
Member

mraleph commented Jul 3, 2024

But my thinking was that it's only when we use force_long_prefix the .. segments cause issues in "absolute paths".

force_long_prefix is just a short-cut in some sense. So whatever problem exists with it, will also exist with paths that are just long by themselves.

@aam
Copy link
Contributor Author

aam commented Jul 3, 2024

Shortcut in what sense? I thought presence of long prefix disables some file/directory name processing and validation - stuff like handling of ..

@mraleph
Copy link
Member

mraleph commented Jul 3, 2024

@aam in the implementation sense. I added force_long_prefix to simplify implementation of Copy which would otherwise need to worry with path length.

copybara-service bot pushed a commit that referenced this issue Jul 3, 2024
Bug: #56125
Change-Id: Ib270d4ad7f7b4570954aa3688edccef0ccbbad85
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/374297
Reviewed-by: Alexander Aprelev <[email protected]>
Commit-Queue: Ryan Macnak <[email protected]>
@mraleph
Copy link
Member

mraleph commented Jul 10, 2024

@aam Would you submit a follow up change to fix the whole issue?

@aam aam self-assigned this Jul 10, 2024
@a-siva a-siva added the triaged Issue has been triaged by sub team label Jul 10, 2024
@aam
Copy link
Contributor Author

aam commented Jul 12, 2024

@mraleph wrote

Would you submit a follow up change to fix the whole issue?

Is this what you had in mind https://dart-review.googlesource.com/c/sdk/+/375421 ?

copybara-service bot pushed a commit that referenced this issue Aug 15, 2024
This ensures that on Windows we take advantage of 32k-character file name limit consistently for all files and directories operations.

TEST=ci
BUG=#56125
BUG=#55972
CoreLibraryReviewExempt: adds windows-specific error code for invalid file name
Change-Id: I83bd3ceac579e589469e47b2cf5216db457cae8b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/375421
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Alexander Aprelev <[email protected]>
@aam aam closed this as completed Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. gardening os-windows triaged Issue has been triaged by sub team
Projects
None yet
Development

No branches or pull requests

3 participants