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: support GNU numeric format. #101172

Merged
merged 8 commits into from
Jun 25, 2024
Merged

Tar: support GNU numeric format. #101172

merged 8 commits into from
Jun 25, 2024

Conversation

tmds
Copy link
Member

@tmds tmds commented Apr 17, 2024

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.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 17, 2024
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.
@tmds
Copy link
Member Author

tmds commented Apr 17, 2024

Test failures are unrelated.

@tmds
Copy link
Member Author

tmds commented Apr 29, 2024

@dotnet/area-system-formats-tar ptal.

@tmds
Copy link
Member Author

tmds commented May 8, 2024

@carlossanlop ptal.

@tmds
Copy link
Member Author

tmds commented May 21, 2024

@dotnet/area-system-formats-tar ptal.

@am11
Copy link
Member

am11 commented May 21, 2024

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! 👍

Copy link
Member

@carlossanlop carlossanlop left a 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.

@carlossanlop
Copy link
Member

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmds
Copy link
Member Author

tmds commented May 28, 2024

@carlossanlop I have addressed your feedback, can you take another look?

Copy link
Member

@carlossanlop carlossanlop left a 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.

@carlossanlop
Copy link
Member

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmds
Copy link
Member Author

tmds commented Jun 6, 2024

@carlossanlop I've addressed the additional feedback except for the writerbig test. See #101172 (comment).

@tmds
Copy link
Member Author

tmds commented Jun 14, 2024

@carlossanlop can you let me know if you want this test included: #101172 (comment)?

@carlossanlop
Copy link
Member

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@carlossanlop carlossanlop left a 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.

@tmds
Copy link
Member Author

tmds commented Jun 17, 2024

@carlossanlop can you start another run of runtime-extra-platforms?

@carlossanlop
Copy link
Member

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmds
Copy link
Member Author

tmds commented Jun 25, 2024

@carlossanlop this is good to merge.

@carlossanlop carlossanlop merged commit c5e8f83 into dotnet:main Jun 25, 2024
104 of 122 checks passed
@carlossanlop
Copy link
Member

Thanks @tmds!

@carlossanlop
Copy link
Member

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?

@ericstj
Copy link
Member

ericstj commented Jul 23, 2024

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.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Formats.Tar community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TarReader throws on archive that other tools accept
4 participants