-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: support GNU numeric format. #101172
Tar: support GNU numeric format. #101172
Conversation
The tar specification stores numeric fields using an octal representation. This limits the range of values that can be stored. To increase the supported range, a GNU extension defines that when the leading byte is 0xff/0x80 the remaining bytes are a negative/positive big endian formatted value. When writing under the PAX format, we continue to only use the only octal representation in the header fields. The values are overridden using extended attributes.
Test failures are unrelated. |
@dotnet/area-system-formats-tar ptal. |
@carlossanlop ptal. |
@dotnet/area-system-formats-tar ptal. |
It would be nice to get this in for .NET 9. The sooner tar implementation matures the better for the ecosystem. Thanks for this @tmds! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change. I left some questions. Haven't gone through the whole PR yet.
We also need to run runtime-extra-platforms tests.
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarReader/TarReader.File.Async.Tests.cs
Show resolved
Hide resolved
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
@carlossanlop I have addressed your feedback, can you take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the feedback. I gave it another pass and left some comments for you to consider.
I'll run runtime-extra-platforms now to see if we catch any unexpected errors.
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarReader/TarReader.File.Async.Tests.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/PosixTarEntry.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/PosixTarEntry.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.Base.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
@carlossanlop I've addressed the additional feedback except for the |
@carlossanlop can you let me know if you want this test included: #101172 (comment)? |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved pending two things:
- The re-addition of the removed writer-big test case as suggested in the snippet.
- I ran runtime-extra-platforms, which executes tar tests in many other platforms (mobile maybe). Please verify we don't see any new failures related to this change.
@carlossanlop can you start another run of runtime-extra-platforms? |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
@carlossanlop this is good to merge. |
Thanks @tmds! |
Considering this is a problem that can be seen in runfo (one of our infrastructure tools) I'm inclined to backport this to .NET 8. Would you have any objections, @ericstj? |
Runfo isn't a critical tool, its just for debugging. I'd instead suggest you seek out input from customers reporting this one. See if anyone is blocked on .NET 8.0 - that's more important. This definitely would meet the bar for servicing if we had a customer blocked by it. |
The tar specification stores numeric fields using an octal representation. This limits the range of values that can be stored.
To increase the supported range, a GNU extension defines that when the leading byte is 0xff/0x80 the remaining bytes are a negative/positive big endian formatted value.
When writing under the PAX format, we continue to only use the only octal representation in the header fields. The values are overridden using extended attributes.
Fixes #93763.
@dotnet/area-system-formats-tar ptal.