Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -374,12 +374,9 @@ private int WritePosixName(Span<byte> buffer)

if (_name.Length > FieldLengths.Name)
{
int prefixBytesLength = Math.Min(_name.Length - FieldLengths.Name, FieldLengths.Name);
Span<byte> remaining = prefixBytesLength <= 256 ?
stackalloc byte[prefixBytesLength] :
new byte[prefixBytesLength];

int encoded = Encoding.ASCII.GetBytes(_name.AsSpan(FieldLengths.Name), remaining);
int prefixBytesLength = Math.Min(_name.Length - FieldLengths.Name, FieldLengths.Prefix);
Span<byte> remaining = stackalloc byte[prefixBytesLength];
int encoded = Encoding.ASCII.GetBytes(_name.AsSpan(FieldLengths.Name, prefixBytesLength), remaining);
Debug.Assert(encoded == remaining.Length);

checksum += WriteLeftAlignedBytesAndGetChecksum(remaining, buffer.Slice(FieldLocations.Prefix, FieldLengths.Prefix));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@

using System.Collections.Generic;
using System.IO;
using System.IO.Enumeration;
using System.Linq;
using Microsoft.DotNet.XUnitExtensions;
using Xunit;

namespace System.Formats.Tar.Tests
Expand Down Expand Up @@ -253,5 +255,56 @@ public void SkipRecursionIntoBaseDirectorySymlink()

Assert.Null(reader.GetNextEntry());
}

[ConditionalTheory(nameof(CanExtractWithTarTool))]
[MemberData(nameof(GetPaxAndGnuTestCaseNames))]
Copy link
Contributor

Choose a reason for hiding this comment

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

We need another condition here confirming that the /usr/bin/tar binary exists. We don't know if some Unix flavors put the binary somewhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

We assume /bin/ln always exists, I thought we could do the same here.

symLinkProcess.StartInfo.FileName = "/bin/ln";

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we will know when the CI is done.

Copy link
Member

Choose a reason for hiding this comment

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

Another option is to assume /usr/bin is on the $PATH in every distro we test on. Just as you're assuming here that C:\Windows\System32 is on the Windows PATH

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, on Alpine, it's implemented by Busybox, with a link. So /usr/bin/tar does not exist, but tar should work:

danmoseL:~# ls -alF /bin/tar
lrwxrwxrwx    1 root     root            12 Mar 31  2021 /bin/tar -> /bin/busybox*

Copy link
Member

Choose a reason for hiding this comment

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

And incidentally, the busybox tar is probably significantly different to the GNU and BSD tar's so it would be good to try it. (You can also do it locally in WSL2 of course if you want)

Copy link
Contributor

@carlossanlop carlossanlop Sep 8, 2022

Choose a reason for hiding this comment

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

As discussed via chat - Some link tests are failing, possibly because the OS does not support the creation of symlinks, so they should get separated into their own tests and decorated with the CanCreateSymbolicLinks condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed slightly different on latest commit:

string[] symlinkTestCases = { "file_symlink", "foldersymlink_folder_subfolder_file", "file_longsymlink" };
if (symlinkTestCases.Contains(testCaseName) && !MountHelper.CanCreateSymbolicLinks)
{
throw new SkipTestException("Symlinks are not supported on this platform.");
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Nvm, this wasn't the reason of the failure, my guess is that Windows Nano Server has a bsdtar version that does not support extraction of symlinks.

Copy link
Member

@danmoseley danmoseley Sep 8, 2022

Choose a reason for hiding this comment

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

Maybe, but won't this fail on any Windows version if MountHelper.CanCreateSymbolicLinks is false? I don't see it being checked in the current commit.

As you know symlinks on Windows require elevation unless you have developer mode. You almost certainly have developer mode enabled on your own machine, but it may not be enabled on others' machines. This won't show up in Helix because I believe they run all tests elevated, but any test we have that creates symlinks mst test MountHelper.CanCreateSymbolicLinks (even testing you're elevated may not be enough, eg ?Nano)

Copy link
Member Author

@jozkee jozkee Sep 8, 2022

Choose a reason for hiding this comment

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

I don't see it being checked in the current commit.

Yeah, I removed it because nano was still failing (even with the CanCreateSymbolicLinks check).
You can see the run for that commit (1b4bf33):
https://github.com/dotnet/runtime/pull/75023/checks?check_run_id=8242739135

Got this error log:
https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-75023-merge-fbe5d4bb79764adabe/System.Formats.Tar.Tests/1/console.da17f2b2.log?helixlogtype=result

Copy link
Member

Choose a reason for hiding this comment

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

I believe you, just saying that if you fix whatever that problem is, you'd still need that check

Copy link
Contributor

Choose a reason for hiding this comment

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

How are we using CanCreateSymbolicLinks in our System.IO symlink tests?

public void ResultingArchive_CanBeExtractedByOtherTools(string testCaseName)
{
if (testCaseName == "specialfiles" && !PlatformDetection.IsSuperUser)
{
throw new SkipTestException("specialfiles is only supported on Unix and it needs sudo permissions for extraction");
}

using TempDirectory root = new TempDirectory();

string archivePath = GetTarFilePath(CompressionMethod.Uncompressed, TestTarFormat.pax.ToString(), testCaseName);
string tarExtractedPath = Path.Join(root.Path, "tarExtracted");
Directory.CreateDirectory(tarExtractedPath);

// extract with /usr/bin/tar
ExtractWithTarTool(archivePath, tarExtractedPath);

// create archive with TarFile
string dotnetTarArchive = Path.Join(root.Path, "dotnetTarArchive.tar");
TarFile.CreateFromDirectory(tarExtractedPath, dotnetTarArchive, includeBaseDirectory: false);

// extract TarFile's archive with /usr/bin/tar
string dotnetTarExtractedPath = Path.Join(root.Path, "dotnetTarExtracted");
Directory.CreateDirectory(dotnetTarExtractedPath);
ExtractWithTarTool(dotnetTarArchive, dotnetTarExtractedPath);

// compare results
Dictionary<string, FileSystemInfo> expectedEntries = GetFileSystemInfosRecursive(tarExtractedPath).ToDictionary(fsi => fsi.FullName.Substring(tarExtractedPath.Length));

foreach (var actualEntry in GetFileSystemInfosRecursive(dotnetTarExtractedPath))
{
string keyPath = actualEntry.FullName.Substring(dotnetTarExtractedPath.Length);

Assert.True(expectedEntries.TryGetValue(keyPath, out FileSystemInfo expectedEntry), "Entry not expected.");
Assert.Equal(expectedEntry.Attributes, actualEntry.Attributes);
Assert.Equal(expectedEntry.LinkTarget, actualEntry.LinkTarget);

expectedEntries.Remove(keyPath);
}

// All expected entries should've been found and removed.
Assert.Equal(0, expectedEntries.Count);
}

private static FileSystemEnumerable<FileSystemInfo> GetFileSystemInfosRecursive(string path)
{
var enumerationOptions = new EnumerationOptions { RecurseSubdirectories = true };
return new FileSystemEnumerable<FileSystemInfo>(path, (ref FileSystemEntry e) => e.ToFileSystemInfo(), enumerationOptions);
}
}
}
33 changes: 33 additions & 0 deletions src/libraries/System.Formats.Tar/tests/TarTestsBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Runtime.CompilerServices;
Expand Down Expand Up @@ -611,5 +612,37 @@ public static IEnumerable<object[]> GetPaxAndGnuTestCaseNames()
yield return new object[] { name };
}
}

public static bool CanExtractWithTarTool = CanExtractWithTarTool_Method();

private static bool CanExtractWithTarTool_Method()
{
using Process tarToolProcess = new Process();
tarToolProcess.StartInfo.FileName = "tar"; // location varies: find it on the PATH.
tarToolProcess.StartInfo.Arguments = "--help";

tarToolProcess.StartInfo.RedirectStandardOutput = true;
tarToolProcess.Start();
Copy link
Member

Choose a reason for hiding this comment

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

this will fail if the test is run on a version of Windows prior to when they added 'tar'. do we have a "find on path" method or existing code we can use? or, we could catch whatever the exception is when the file does not exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to this: https://docs.microsoft.com/en-us/virtualization/community/team-blog/2017/20171219-tar-and-curl-come-to-windows

Beginning in Insider Build 17063, we’re introducing two command-line tools to the Windows toolchain: curl and bsdtar

I assume they're talking about Windows 11. Interestingly, my Windows 10 machine also has tar available, so I think the document is incomplete.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Windows 10, executing tar -h on PowerShell gets me this:

bsdtar 3.5.2 - libarchive 3.5.2 zlib/1.2.5.f-ipp

And on Windows 11:

bsdtar 3.5.2 - libarchive 3.5.2 zlib/1.2.5.f-ipp bz2lib/1.0.6 

Copy link
Member

Choose a reason for hiding this comment

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

Decoder .. this is Windows 10 RS4, in 2018.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good to know. The article's date is 04/25/2022, that's why I was assuming Win11.

Copy link
Member

Choose a reason for hiding this comment

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

Also note we support Windows 10 from RS1 (1607)

Copy link
Member Author

Choose a reason for hiding this comment

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

@danmoseley if it fails, it will just skip the test, something like this:

    Discovering: System.Formats.Tar.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Formats.Tar.Tests (found 1 of 578 test case)
    Starting:    System.Formats.Tar.Tests (parallel test collections = on, max threads = 12)
      System.Formats.Tar.Tests.TarFile_CreateFromDirectory_File_Tests.ResultingArchive_CanBeExtractedByOtherTools [SKIP]
        Condition(s) not met: "CanExtractWithTarTool (TargetInvocationException)"
    Finished:    System.Formats.Tar.Tests
  === TEST EXECUTION SUMMARY ===
     System.Formats.Tar.Tests  Total: 1, Errors: 0, Failed: 0, Skipped: 1, Time: 0.172s

Copy link
Member Author

Choose a reason for hiding this comment

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

I will wrap the code in a try-catch, just in case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now I wonder, if that's how xUnit handles exceptions on expressions used for ConditionalFact/TheoryAttributes, does it mean that we can end up with tests no longer running and we will never notice?
cc @dotnet/area-infrastructure-libraries


tarToolProcess.StandardOutput.ReadToEnd();
tarToolProcess.WaitForExit();

return (tarToolProcess.ExitCode == 0);
}

public static void ExtractWithTarTool(string source, string destination)
{
using Process tarToolProcess = new Process();
tarToolProcess.StartInfo.FileName = "tar"; // location varies: find it on the PATH.
tarToolProcess.StartInfo.Arguments = $"-xf {source} -C {destination}";
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add an additional boolean parameter isCompressed to this ExtractWithTarTool method. If true, then the arguments could include the z letter, which is used to extract *.tar.gz files.

We would just have to adjust line 271 of the ResultingArchive_CanBeExtractedByOtherTools test method, so that it takes a variable CompressionMethod.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that should be part of the TODOs about adding more variants of this test.


tarToolProcess.StartInfo.RedirectStandardOutput = true;
tarToolProcess.Start();

tarToolProcess.StandardOutput.ReadToEnd();
tarToolProcess.WaitForExit();

Assert.Equal(0, tarToolProcess.ExitCode);
}
}
}