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

Tar: add executable_bit_only support. #17622

Closed
wants to merge 4 commits into from

Conversation

Rexicon226
Copy link
Contributor

@Rexicon226 Rexicon226 commented Oct 19, 2023

Closes: #17462

Still working on polishing this and seeing if there any any issues with this. To be honest, I'm just doubting that it's this simple, so I need some time to make sure.

Trying it on something like ci folder, it seems to be working.
image

if I'm missing something super obvious please let me know.

@Rexicon226
Copy link
Contributor Author

Rexicon226 commented Oct 19, 2023

Would it make sense to change the properties of the file, after it has been confirmed to successfully been written?

@Rexicon226 Rexicon226 marked this pull request as ready for review October 19, 2023 19:52
@ianprime0509
Copy link
Contributor

From what I understand, the more difficult part of #17462 is implementing the necessary support for Windows using ACLs (the last part of the referenced TODO comment). I'm not familiar with Windows development, so I'm not sure specifically what ACL is being referred to or how to interact with it using Windows APIs, however.

While completely ignoring the executable bit on Windows is simpler to implement, it would cause problems when #17463 is implemented, because it would cause package hashes to differ on Windows vs other platforms: the executable bit would be considered in the hash on POSIX platforms but unavailable on Windows. (This will also be an issue on WASI, but it looks like WASI preview 2 is intended to support file modes similar to POSIX at some point: https://github.com/WebAssembly/wasi-filesystem/blob/d86efbe6c8b7590cfc1306c7bb02dbf960041434/wit/types.wit#L145-L157)

@Rexicon226
Copy link
Contributor Author

So, Windows ACL, from what I understand, is quite a lot different from other ACL implementations. ACL standing for "Access-control List". It's just a general term for a file permissions "format" I guess you could call it.

Here is good article I found for analyzing the Windows ACL using the win32api here. And this one for creating and modifying them here. I've never delved deep into the Windows file system methods, but they seem quite interesting and doable. I'm going to spend a couple days exploring implementations for ACL control on windows. Current std simply forces things like "chmod()" to just accept u0, and are useless.

@andrewrk
Copy link
Member

andrewrk commented Oct 24, 2023

std.fs.File.chmod should be deleted, since std.os (to be renamed to std.posix in #5019) has the API and it is a POSIX-only API. And then std.fs.File can be augmented with cross-platform API, if possible.

Either way, std.os.windows can will have the Windows-specific API, and the cross-platform abstraction will either go into std.tar or into std.fs.File.

@Rexicon226
Copy link
Contributor Author

Makes sense. I will make a draft pr to track when I have something. Once the reformat of std.fs.File is done, I'll come back to this PR and re-implement it with the "new" api.

@andrewrk
Copy link
Member

Needs a test, needs a rebase on the updated tar implementation. If you want to continue this work please open a new PR.

@andrewrk andrewrk closed this Mar 14, 2024
@Rexicon226 Rexicon226 deleted the exec-bit branch April 1, 2024 16:51
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.

std.tar: implement executable_bit_only mode
3 participants