Skip to content

[AssetMapper] fix tests on Windows#58152

Merged
xabbuh merged 1 commit intosymfony:7.2from
xabbuh:pr-57890
Sep 3, 2024
Merged

[AssetMapper] fix tests on Windows#58152
xabbuh merged 1 commit intosymfony:7.2from
xabbuh:pr-57890

Conversation

@xabbuh
Copy link
Copy Markdown
Member

@xabbuh xabbuh commented Sep 3, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

@carsonbot carsonbot added this to the 7.2 milestone Sep 3, 2024
@xabbuh
Copy link
Copy Markdown
Member Author

xabbuh commented Sep 3, 2024

There is a temp file created that also needs to be removable when cleaning up in the tearDown() method: https://ci.appveyor.com/project/fabpot/symfony/builds/50521173#L1064

@xabbuh xabbuh merged commit 1958d21 into symfony:7.2 Sep 3, 2024
@xabbuh xabbuh deleted the pr-57890 branch September 3, 2024 07:52
@smnandre
Copy link
Copy Markdown
Member

smnandre commented Sep 3, 2024

Thank you @xabbuh this test really broke my mind (#58141 (review))

I'm not sure exacty "when" this temp file is created... my guts say there is something wrong with the /.! format that mess with the Finder list of files, and we end up trying to remove a directory before its content, but until now my guts have been wrong on this so ... 😅

Notes i made trying to debug this:

1) Symfony\Component\AssetMapper\Tests\ImportMap\RemotePackageStorageTest::testSaveThrowsWhenFailing
Symfony\Component\Filesystem\Exception\IOException: Failed to remove file "C:\Users\appveyor\AppData\Local\Temp\1/.!!8wd\assets\acme\vendor\module_specifier\modF8A2.tmp": unlink(C:\Users\appveyor\AppData\Local\Temp\1/.!!8wd\assets\acme\vendor\module_specifier\modF8A2.tmp): Permission denied
C:\projects\symfony\src\Symfony\Component\Filesystem\Filesystem.php:197
C:\projects\symfony\src\Symfony\Component\Filesystem\Filesystem.php:185
C:\projects\symfony\src\Symfony\Component\Filesystem\Filesystem.php:185
C:\projects\symfony\src\Symfony\Component\Filesystem\Filesystem.php:185
C:\projects\symfony\src\Symfony\Component\Filesystem\Filesystem.php:185
C:\projects\symfony\src\Symfony\Component\Filesystem\Filesystem.php:185
C:\projects\symfony\src\Symfony\Component\Filesystem\Filesystem.php:153
C:\projects\symfony\src\Symfony\Component\AssetMapper\Tests\ImportMap\RemotePackageStorageTest.php:35
--

So the exception is thrown during the test teardown (RemotePackageStorageTest.php)

symfony/src/Symfony/Component/AssetMapper/Tests/ImportMap/RemotePackageStorageTest.php

Lines 33 to 36 in cc11de0

protected function tearDown(): void
{
$this->filesystem->remove(self::$writableRoot);
}

This method calls the Filesystem remove() method (with a directory path)

symfony/src/Symfony/Component/Filesystem/Filesystem.php

Line 153 in cc11de0

self::doRemove($files, false);

The remove then call the doRemove with the directory and recursive "false"

And after some iterations of

symfony/src/Symfony/Component/Filesystem/Filesystem.php

Lines 184 to 185 in cc11de0

$filesystemIterator = new \FilesystemIterator($file, \FilesystemIterator::CURRENT_AS_PATHNAME | \FilesystemIterator::SKIP_DOTS);
self::doRemove(iterator_to_array($filesystemIterator, true), true);

We arrive at the final exception

symfony/src/Symfony/Component/Filesystem/Filesystem.php

Lines 196 to 198 in cc11de0

} elseif (!self::box('unlink', $file) && ((self::$lastError && str_contains(self::$lastError, 'Permission denied')) || file_exists($file))) {
throw new IOException(\sprintf('Failed to remove file "%s": ', $file).self::$lastError);
}

So if you have any way to know precisely when it is created i'd be very interested to know

@smnandre smnandre mentioned this pull request Sep 3, 2024
Merged
@xabbuh
Copy link
Copy Markdown
Member Author

xabbuh commented Sep 5, 2024 •
edited
Loading

I just had another look and found the root issue to be located in the Filesystem component. In the dumpFile() method a temporary file is used which reuses the permissions of the target file leading to it not being removed on Windows filesystems afterwards if it has no write permissions: #58185

@xabbuh xabbuh mentioned this pull request Sep 5, 2024
Merged
nicolas-grekas added a commit that referenced this pull request Sep 16, 2024
@nicolas-grekas
bug #58185 [Filesystem] make sure temp files can be cleaned up on Win…
56fa4dc 
…dows (xabbuh)

This PR was merged into the 5.4 branch.

Discussion
----------

[Filesystem] make sure temp files can be cleaned up on Windows

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

based on the findings while debugging the AppVeyor failures that we worked around in #58152

Commits
-------

a832b67 make sure temp files can be cleaned up on Windows
@xabbuh xabbuh mentioned this pull request Sep 18, 2024
Merged
@xabbuh
Copy link
Copy Markdown
Member Author

xabbuh commented Sep 18, 2024

so we can revert this now: #58302

nicolas-grekas added a commit that referenced this pull request Sep 18, 2024
@nicolas-grekas
minor #58302 [Filesystem] revert test changes (xabbuh)
3711a6a 
This PR was merged into the 7.2 branch.

Discussion
----------

[Filesystem] revert test changes

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

we can revert #58152 now that the underlying bug was fixed with #58185

Commits
-------

5ea44be revert test changes
fabpot added a commit that referenced this pull request Sep 27, 2024
@fabpot
feature #58141 [AssetMapper] Search & filter assets in `debug:asset-m…
1b13c33 
…apper` command (smnandre)

This PR was squashed before being merged into the 7.2 branch.

Discussion
----------

[AssetMapper] Search & filter assets in `debug:asset-mapper` command

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix #...
| License       | MIT

* Add `name` optional argument to filter paths & assets
* Add `--ext` option to filter assets by extension (e.g "css")
* Add `--vendor` option to only show assets from vendor packages (and `--no-vendor` to hide them)
* Update command help

Update:
~~* skip flappy test using chmod on Windows~~ (done by `@xabbuh` in #58152)

Commits
-------

8806e24 [AssetMapper] Search & filter assets in `debug:asset-mapper` command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexandre-daubois alexandre-daubois alexandre-daubois approved these changes

Assignees

No one assigned

Labels

AssetMapper Status: Reviewed

Projects

None yet

Milestone

7.2

Development

Successfully merging this pull request may close these issues.

4 participants

@xabbuh @smnandre @alexandre-daubois @carsonbot