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

fix: preserve file permission #362

Merged
merged 2 commits into from
Dec 21, 2021
Merged

fix: preserve file permission #362

merged 2 commits into from
Dec 21, 2021

Conversation

rubiagatra
Copy link
Contributor

Close #156

This is only draft if someone can have discussion with me it would be great

@vercel
Copy link

vercel bot commented Dec 19, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vercel/turbo-site/FBiFEm5tcwgBgrfirxfTVP1Gn2aQ
✅ Preview: https://turbo-site-git-fork-rubiagatra-fix-preserved-file-permission.vercel.sh

Copy link
Contributor

@jaredpalmer jaredpalmer left a comment

Choose a reason for hiding this comment

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

Can you change the call sites to pass 0 to preserve permissions?

err := fs.RecursiveCopyOrLinkFile(cachedFolder, target, fs.DirPermissions, true, true)

if err := fs.CopyOrLinkFile(file, filepath.Join(f.cacheDirectory, hash, rel), fs.DirPermissions, fs.DirPermissions, true, true); err != nil {

and in all of this file

https://github.com/vercel/turborepo/blob/a229aaa1c4aaecaef054adf09a8800ca74e4c1c6/cli/internal/prune/prune.go#L175-L291

and lastly here:

if err := fs.CopyFile(chrometracing.Path(), name, fs.DirPermissions); err != nil {

@rubiagatra
Copy link
Contributor Author

rubiagatra commented Dec 21, 2021

I just did that you requested @jaredpalmer and undo my draft commit.

  1. Pass 0 instead of fs.DirPermissions
  2. Run go test ./...
?   	turbo/cmd/turbo	[no test files]
?   	turbo/internal/api	[no test files]
?   	turbo/internal/backends	[no test files]
?   	turbo/internal/backends/nodejs	[no test files]
?   	turbo/internal/cache	[no test files]
?   	turbo/internal/client	[no test files]
?   	turbo/internal/config	[no test files]
ok  	turbo/internal/context	1.608s
ok  	turbo/internal/core	0.533s
ok  	turbo/internal/fs	1.295s
?   	turbo/internal/globby	[no test files]
?   	turbo/internal/info	[no test files]
?   	turbo/internal/login	[no test files]
ok  	turbo/internal/logstreamer	0.841s
?   	turbo/internal/prune	[no test files]
ok  	turbo/internal/run	2.290s
?   	turbo/internal/scm	[no test files]
?   	turbo/internal/ui	[no test files]
ok  	turbo/internal/ui/term	1.980s
ok  	turbo/internal/util	1.140s
?   	turbo/internal/util/browser	[no test files]
?   	turbo/internal/xxhash	[no test files]

Is there anything I can help with my changes to not break something?. I see potential test/documentation/onboarding improvement in the future, I am happy to help.

@rubiagatra rubiagatra changed the title draft: preserved file permission bugfix: preserved file permission Dec 21, 2021
@jaredpalmer jaredpalmer changed the title bugfix: preserved file permission fix: preserved file permission Dec 21, 2021
@jaredpalmer jaredpalmer changed the title fix: preserved file permission fix: preserve file permission Dec 21, 2021
@jaredpalmer jaredpalmer self-requested a review December 21, 2021 14:29
@jaredpalmer jaredpalmer merged commit 67e64ab into vercel:main Dec 21, 2021
jaredpalmer added a commit that referenced this pull request Dec 22, 2021
@jaredpalmer
Copy link
Contributor

This unfortunately is more involved change. The current implementation breaks binary executables

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File Permissions not Preserved from Cache
2 participants