-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Implement Tar Global Extended Attributes API changes #70869
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsAddresses the rest of the APIs approved in: #69935 Depends on the conversion constructors PR: #70325 While I merge that other PR, this one can be reviewed by looking at the 3 individual commits (look at that pretty separator commit 🙂):
|
src/libraries/System.Formats.Tar/src/System/Formats/Tar/PaxGlobalExtendedAttributesTarEntry.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarReader/TarReader.File.Tests.Base.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarReader/TarReader.GetNextEntry.Tests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated
Show resolved
Hide resolved
7fcc8bb
to
27c1734
Compare
Force pushing a rebase merge to fix merge conflicts with main. |
27c1734
to
9888a13
Compare
9888a13
to
aa9c213
Compare
/azp run runtime-extra-platforms |
1 similar comment
/azp run runtime-extra-platforms |
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
{ | ||
Debug.Assert(globalExtendedAttributesEntryNumber >= 1); | ||
|
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.
As an aside - the code below reading TMPDIR then falling back to /tmp- why doesn't it just call Path.GetTempPath() that does the same thing?
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.
Good suggestion. I'll use Path.GetTempPath
.
Here's why I wasn't using that:
The GNU tar manual specifies how the global extended attribute header name field obtains its value:
8.3.7.1 Controlling Extended Header Keywords
$TMPDIR/GlobalHead.%p.%n
[...]
$TMPDIR
stands for the value of theTMPDIR
environment variable. IfTMPDIR
is not set, tar uses/tmp
.
This description looks very similar to the remarks section of Path.GetTempPath:
Remarks
This method checks for the existence of environment variables in the following order and uses the first path found:
Linux
- The path specified by the TMPDIR environment variable. If the path is not specified in the TMPDIR environment variable, the default path /tmp/ is used.
Windows
- The path specified by the TMP environment variable.
- The path specified by the TEMP environment variable.
- The path specified by the USERPROFILE environment variable.
- The Windows directory.
So a couple of things need to be considered if we are to use Path.GetTempPath
:
- I don't see why any tool would be reading the GEA path, or having reading/writing behavior depend on it. The field isn't very interesting, unless the user cares about the process ID or the sequence number.
- Keep in mind that none of the tar specs (at least the ones I read) specify what the expected behavior for Windows should be. Everything is Unix focused. If we format that string embedding Windows paths, I hope other tools don't break due to finding unexpected drive names or
\
separators in that field. But go back to the previous point: I don't think any tool should make its behavior depend on this field. - If the Windows path ends up being larger than than 100 bytes, the sequence number and process ID would get truncated. Which is fine, I guess, because a couple of lines below, I re-generate the string by forcing the usage of
/tmp
.
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.
Overall LGTM, the only thing I would like to know before hitting the accept button is why the code is using the TMPDIR
env var rather that calling the Path
API? (issue initally raised by Dan)
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.cs
Outdated
Show resolved
Hide resolved
"00000000001111111111222222222233333333334444444444555555555566666666667777777777888888888899999999/"); | ||
|
||
TarEntry file = reader.GetNextEntry(); | ||
VerifyRegularFileEntry(file, format, | ||
$"00000000001111111111222222222233333333334444444444555555555566666666667777777777888888888899999999/00000000001111111111222222222233333333334444444444555555555566666666667777777777888888888899999.txt", |
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.
nit: it's hard to verify the long string lengths, perhaps you could use the string ctor that accepts char count and just repeat a single char? Example:
- "00000000001111111111222222222233333333334444444444555555555566666666667777777777888888888899999999/"
+ new string('0', 98) + "/"
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.
There are two reasons for these long strings:
- The tar assets from dotnet/runtime-assets already use these strings for long directory names and filenames, so changing these would mean changing the assets first, then bump the package version number in this PR.
- When I was starting to test long names, I discovered it was very annoying to not be able to determine where exactly the string was getting unexpectedly truncated, or causing a failure. Having groups of 10 equal characters made it much easier to track the exact location.
/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.
LGTM, thank you @carlossanlop !
Addresses the rest of the APIs approved in: #69935
The other half of the APIs got merged here: #70325