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.

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

Handle symlinks properly #84

Merged

Conversation

doekenorg
Copy link

Currently the logic to resolve symlinks is flawed making Strauss ignore symlinked packages. This PR solves that problem, and adds a test to ensure the original symlink is intact after running Strauss with "delete_vendor_packages": true.

Thanks for providing this tool. If this PR needs changing let me know.

I think this PR can finish #64.

Ref: #64 (comment)

@BrianHenryIE
Copy link
Owner

Hey. I made a release today which was related to another issue and I haven't properly looked at this yet.

I would think that when deleteVendorPackages is true the symlink should be removed: 503f110

Does that contradict where you say

original symlink is intact

@doekenorg
Copy link
Author

doekenorg commented Feb 16, 2024

Hey sorry for the confusion. The thing is, if I have that setting on, the original folder (from which it is symlinked) gets cleared out. And that shouldn't happen. The symlink can be deleted. But the original files should stay intact. Does that make sense?

My plugin is located in ~/my-plugin. It uses ~/dependency as a aymlinked dependency. For me it clears out ~/dependency instead of just removing ~/plugin/vendor/namespace/dependency.

This fix makes sure it can find the location properly, when using relative paths. The test provides the use case that didn't work for me properly.

Ps, this worked for me from the PR version; but I couldn't figure out how to package that to a phar, and try that file as well. Could you help with that so I can make 100% sure this solves my problem?

@BrianHenryIE
Copy link
Owner

Create the phar (from GitHub Actions workflow):

composer install --no-dev --prefer-dist;
wget -O phar-composer.phar https://github.com/clue/phar-composer/releases/download/v1.4.0/phar-composer-1.4.0.phar;
mkdir build;
mv vendor build/vendor;
mv src build/src;
mv bin build/bin;
mv composer.json build;
php -d phar.readonly=off phar-composer.phar build ./build/;

Clean up:

mv ./build/*.* .
mv ./build/* .
rm -rf build
composer install

Run it:

chmod +x strauss.phar
php strauss.phar

TBH, I don't know what --prefer-dist is doing there, I'd expect that to be the default. But it was in the command from wherever I copied it from, so maybe it's important!

Those instructions don't include chmod +x phar-composer.phar. Let me know if that step is needed please.

I should document this somewhere better.

@doekenorg
Copy link
Author

doekenorg commented Feb 29, 2024

@BrianHenryIE Cool, I was finally able to test this solution and it works! It no longer clears out the folders!

When using the normal version I get a Warning: rmdir(/Users/doekenorg/code/.../vendor/...): Not a directory in... error. This is because it resolves the path incorrectly; and doesn't mark it as a symlink. So I can confirm the solution here works; as the test case also shows 👍

Edit: Funny enough I did find a small bug when a package configures an autoload path without a directory separator. Adjusted for that. So to sum up:

  • Folders that are symlinked relatively (eg. ../other-package) were not resolved properly
  • Resulting in the attempted removal of the resolved path (so the folder that is symlinked to)
  • Which cleaned out the symlinked package, instead of removing only the symlink.

@BrianHenryIE BrianHenryIE merged commit fa56a30 into BrianHenryIE:master Mar 19, 2024
@doekenorg doekenorg deleted the issue/64-removal-of-symlink-content branch March 19, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants