dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.04k stars 1.55k forks source link

File copying on Windows buildbots is broken #56125

Closed aam closed 1 week ago

aam commented 1 month ago

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 commented 1 month ago

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

aam commented 1 month ago

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

aam commented 1 month ago

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

mraleph commented 1 month ago

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 commented 1 month ago

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

mraleph commented 1 month ago

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 commented 1 month ago

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

mraleph commented 1 month ago

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

mraleph commented 1 month ago

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

aam commented 1 month ago

@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 ?