Skip to content
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

Improve copying of PHP and legacy CLI files #94

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improve copying of PHP and legacy CLI files #94

wants to merge 3 commits into from

Conversation

pjcdawkins
Copy link
Contributor

@pjcdawkins pjcdawkins commented Aug 4, 2023 •
edited
Loading

This checks the existing PHP, legacy CLI and config.yaml files on every run, and re-copies them if necessary.

  • Check if the files were corrupted: for each, check the length and then compare the in-memory bytes against the end of each file (up to 32 KB). The exception is for PHP on Windows, because Zip uncompressing is a little slow: for each file in the Zip, just check the file size using the header (f.UncompressedSize64), and then overwrite if the size does not match.
  • Guard against interrupted file copy: when writing the file, write a .tmp file first and rename it.
  • These file checks happen concurrently.
  • This PR also checks the integrity of the Windows Zip file download during build.
  • Prevent issues on multiple-user (Linux/Unix/MacOS) systems by moving the cache from /tmp to the user's home directory - as determined by os.UserCacheDir() .
  • Stop deleting the files on error, which prevents Phar class loading errors when a running Phar's file has disappeared.

Manual tests:

  • Ubuntu
  • Windows 11
  • Fedora
  • Arch
  • MacOS (amd64 + arm64)

Closes #92
Closes #47

@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch 2 times, most recently from 042e5b7 to f19ce9d Compare August 4, 2023 08:38
@pjcdawkins
Copy link
Contributor Author

@akalipetis this will conflict with the vendorization work - but I was playing with it anyway, to find an efficient way to check the files

@akalipetis akalipetis self-requested a review August 7, 2023 13:39
akalipetis
akalipetis requested changes Aug 23, 2023
View reviewed changes
Copy link
Member

@akalipetis akalipetis left a comment

Choose a reason for hiding this comment

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

The main issues in here are:

  1. There's no need to include a new library (afero) for abstracting the filesystem, we can use fs.FS instead
  2. The hash should be computed from the actual file, not from a file containing the hash value - this beats the purpose of checking hashes in the first place

Makefile Outdated Show resolved Hide resolved
internal/file/file.go Outdated Show resolved Hide resolved
internal/file/file.go Outdated Show resolved Hide resolved
internal/file/file.go Outdated Show resolved Hide resolved
internal/file/file.go Outdated Show resolved Hide resolved
@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch from 1209f44 to f0a1218 Compare August 23, 2023 19:00
@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch 3 times, most recently from e16a11a to 437f559 Compare December 10, 2024 10:48
@pjcdawkins
Copy link
Contributor Author

pjcdawkins commented Dec 10, 2024 •
edited
Loading

Re.

There's no need to include a new library (afero) for abstracting the filesystem, we can use fs.FS instead

fs.FS is read only, and ideally we would test writing files, which afero would support.

But, we could drop all of the testable FS stuff and just write real temporary files with os.

@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch 20 times, most recently from 8bef9a8 to a8aa93c Compare December 10, 2024 23:54
@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch 5 times, most recently from bf4e77c to 793f121 Compare January 11, 2025 08:11
@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch 2 times, most recently from ab42fb3 to da20e47 Compare January 13, 2025 14:25
pjcdawkins
pjcdawkins commented Jan 13, 2025
View reviewed changes
internal/config/dir.go Outdated Show resolved Hide resolved
@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch 3 times, most recently from 4db1dae to 42f9ff8 Compare January 15, 2025 11:22
akalipetis
akalipetis reviewed Jan 15, 2025
View reviewed changes
Copy link
Member

@akalipetis akalipetis left a comment

Choose a reason for hiding this comment

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

Just one last comment, passing around the cacheDir seems not needed and error prone, is there a reason we went for that?

Apart from that, the code LGTM

internal/legacy/php_windows.go Outdated Show resolved Hide resolved
internal/legacy/php_windows.go Outdated Show resolved Hide resolved
@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch 3 times, most recently from 2eb70bd to f6c29b6 Compare January 15, 2025 17:09
This was referenced Jan 15, 2025
Closed
Open
@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch 3 times, most recently from 9dc8b8d to f2d3fd0 Compare January 15, 2025 20:35
@pjcdawkins pjcdawkins changed the title Fix caching of PHP and legacy CLI files Improve copying of PHP and legacy CLI files Jan 15, 2025
@pjcdawkins pjcdawkins force-pushed the compare-file-hashes branch 3 times, most recently from ede0ef2 to 4a8ddcf Compare January 18, 2025 18:16
@pjcdawkins
Refactor legacy CLI invocation
3822243 
- Move 'init' responsibility to the legacy package
- Remove unnecessary Debug on wrapper
- Remove --phar-path option, but preserve {ENV_PREFIX}PHAR_PATH variable
- Remove unnecessary global vars
- Add a test
@pjcdawkins
Improve copying of PHP and legacy CLI files
d458b21 
- Check the files on each run
- Try to avoid partial writes
- Move temporary directory from /tmp to the home directory
@pjcdawkins
Add phpManager type to enforce OS-specific interface
75d4d8c
@pjcdawkins pjcdawkins mentioned this pull request Jan 18, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akalipetis akalipetis akalipetis requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@pjcdawkins @akalipetis