Skip to content

[AssetMapper] fix tests on Windows #58152

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 3, 2024
Merged

[AssetMapper] fix tests on Windows #58152

merged 1 commit into from
Sep 3, 2024

Conversation

xabbuh
Copy link
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
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
8 of 10 checks passed
@xabbuh xabbuh deleted the pr-57890 branch September 3, 2024 07:52
@smnandre
Copy link
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
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
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