Resumbmit tar feedback changes, include Android build fix by carlossanlop · Pull Request #69469 · dotnet/runtime

carlossanlop · 2022-05-18T00:48:56Z

Submitting the changes approved in this PR: #69107

Addresses: #68230

Include an extra commit addressing the android build failure: The method getgrgid_r is not recognized in Android API level < 24, so the build fails, causing widespread CI failures in PRs and in runtime-extra-platforms. The getgrgid_r method is consumed inside the new Tar APIs to retrieve the group name of the specified group Id. The code checks if getgrgid_r is not available, and then attempt to use getgrgid instead as the fallback, which is not thread-safe, but should be available in older Android API levels.

ghost · 2022-05-18T00:49:04Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Submitting the changes approved in this PR: #69107

Addresses: #68230

Include an extra commit addressing the android build failure: The method getgrgid_r is not recognized in Android, so the build fails, causing widespread CI failures in PRs and in runtime-extra-platforms. The getgrgid_r method is consumed inside the new Tar APIs to retrieve the group name of the specified group Id.

An alternative fix would be to check if getgrgid_r is not available, and attempt to use getgrgid instead as the fallback, which is not thread-safe.

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-System.IO`
Milestone:	7.0.0

carlossanlop · 2022-05-18T00:50:51Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-18T00:51:03Z

Azure Pipelines successfully started running 1 pipeline(s).

carlossanlop · 2022-05-18T00:52:45Z

The tests that verify group name in the tar archives will fail in android, so I need to address that before merging.

Edit: As of the latest commit, this is no longer valid. The tests should pass in all platforms because those that don't support the _r method should fallback to the non-_r method.

adamsitnik

Overall it LGTM, but we also need to disable the Android tests that are using the unavailable API

danmoseley · 2022-05-18T11:22:35Z

Does any mono code use getgrgid on Android ? (It should probably be fixed if it does)

carlossanlop · 2022-05-18T21:17:47Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-18T21:18:02Z

Azure Pipelines successfully started running 1 pipeline(s).

eerhardt

Assuming CI is green. This looks good.

carlossanlop · 2022-05-19T05:45:10Z

I am 3 distinct tar tests failing in runtime-extra-platforms:

A) net7.0-windows-Release-x64-CoreCLR_release-(Windows.Server.Core.2004.Amd64.Open)windows.10.amd64.server20h2.open@mcr.microsoft.com/dotnet-buildtools/prereqs:windowsservercore-2004-helix-amd64-20200904200251-272704c:

Unrelated to tar. The test successfully copies all the necessary assets, but then fails to establish communication with the docker container after starting it:

Details

2022-05-18T23:39:18.468Z	INFO helixio(27)	copy_tree_to	Copying 'C:\h\w\AC8F09AE\w\B2C90998\u\unarchived\many_small_files\9\5.txt' to 'C:\h\w\AC8F09AE\w\B2C90998\e\unarchived\many_small_files\9\5.txt'
2022-05-18T23:39:18.468Z	INFO helixio(27)	copy_tree_to	Copying 'C:\h\w\AC8F09AE\w\B2C90998\u\unarchived\many_small_files\9\6.txt' to 'C:\h\w\AC8F09AE\w\B2C90998\e\unarchived\many_small_files\9\6.txt'
2022-05-18T23:39:18.484Z	INFO helixio(27)	copy_tree_to	Copying 'C:\h\w\AC8F09AE\w\B2C90998\u\unarchived\many_small_files\9\7.txt' to 'C:\h\w\AC8F09AE\w\B2C90998\e\unarchived\many_small_files\9\7.txt'
2022-05-18T23:39:18.484Z	INFO helixio(27)	copy_tree_to	Copying 'C:\h\w\AC8F09AE\w\B2C90998\u\unarchived\many_small_files\9\8.txt' to 'C:\h\w\AC8F09AE\w\B2C90998\e\unarchived\many_small_files\9\8.txt'
2022-05-18T23:39:18.484Z	INFO helixio(27)	copy_tree_to	Copying 'C:\h\w\AC8F09AE\w\B2C90998\u\unarchived\many_small_files\9\9.txt' to 'C:\h\w\AC8F09AE\w\B2C90998\e\unarchived\many_small_files\9\9.txt'
2022-05-18T23:39:18.484Z	INFO executor(112)	run	Successfully downloaded work item payload
2022-05-18T23:39:18.484Z	INFO interval(67)	set_abort_after_timer	Set the timer's abort_after to 960 seconds from now: abort_after = 2022-05-18 23:55:18.484122
2022-05-18T23:39:18.577Z	INFO dockerhelper(35)	cleanup	Cleaning up Docker on the machine
2022-05-18T23:39:18.591Z	INFO dockerhelper(52)	pull	Pulling image dotnet-buildtools/prereqs:windowsservercore-2004-helix-amd64-20200904200251-272704c from registry mcr.microsoft.com...
2022-05-18T23:39:18.685Z	INFO dockerhelper(69)	pull	Pull successful.
2022-05-18T23:39:18.686Z	INFO dockerhelper(274)	write_commands_to_file	Generating Docker execution script
2022-05-18T23:39:18.687Z	INFO dockerhelper(170)	run	Starting docker container...
2022-05-18T23:39:18.687Z	INFO interval(67)	set_abort_after_timer	Set the timer's abort_after to 960 seconds from now: abort_after = 2022-05-18 23:55:18.687487
2022-05-18T23:39:19.720Z	ERROR dockerhelper(233)	run	API Error attempting to talk to Docker Server:500 Server Error: Internal Server Error ("CreateComputeSystem 2f74ab29e29394d817342df0834252b05c973211c923f4625757ec064105b9aa: The container operating system does not match the host operating system.

B) net7.0-tvOSSimulator-Release-x64-Mono_Release-OSX.1015.Amd64.Open and
C) net7.0-iOSSimulator-Release-x64-Mono_Release-OSX.1015.Amd64.Open:

All the tests that call the new P/Invoke GetUserName are failing in tvOS and iOS with "No such file or directory" (does not say which errno). I cannot locally build for these platforms, so I'm going to assume we have a similar problem of not having the _r method available on the mobile device, so we need the fallback. I cannot find information that confirms the minimum version where this method is available, but I'll test it with a new commit. Interestingly, I found HAVE_GETPWUID_R checks in mono and coreclr.

Details

Exception messages: System.IO.IOException : No such file or directory Exception stack traces: at Interop.Sys.GetUserName(UInt32 uid)
at System.Formats.Tar.TarWriter.ReadFileFromDiskAndWriteToArchiveStreamAsEntry(String fullPath, String entryName)

carlossanlop · 2022-05-21T20:55:59Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-21T20:56:21Z

Azure Pipelines successfully started running 1 pipeline(s).

carlossanlop · 2022-05-31T04:27:34Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-31T04:27:46Z

Azure Pipelines successfully started running 1 pipeline(s).

radical · 2022-05-31T15:40:20Z

@carlossanlop you can surface files to the wasm app by using <WasmFilesToIncludeInFileSystem Include="..." />. And TargetPath attribute to use a custom path for the wasm side.

carlossanlop · 2022-05-31T16:52:23Z

/azp run runtime-extra-platforms

carlossanlop · 2022-05-31T16:53:35Z

@eerhardt as discussed offline, I removed any new changes related to Browser from this PR and I will address them separately in #69953

@radical thanks for your suggestion. I'll keep it in mind for the PR I will submit for #69953.

eerhardt · 2022-05-31T17:59:09Z

I removed any new changes related to Browser from this PR

I'm still seeing "Browser" related changes in this PR. Specifically src\System.Formats.Tar.csproj

carlossanlop · 2022-05-31T23:58:48Z

I see why. It was part of the previously approved PR. Fixed with the last commit.

carlossanlop · 2022-05-31T23:58:54Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-05-31T23:59:07Z

Azure Pipelines successfully started running 1 pipeline(s).

eerhardt

Once my last 2 comments are resolved, this LGTM

eerhardt

Assuming CI passes,

carlossanlop added the area-System.IO label May 18, 2022

carlossanlop added this to the 7.0.0 milestone May 18, 2022

carlossanlop requested review from adamsitnik, eerhardt, stephentoub, steveisok and tmds May 18, 2022 00:48

carlossanlop self-assigned this May 18, 2022

carlossanlop commented May 18, 2022
View reviewed changes

Comment threadsrc/native/libs/Common/pal_config.h.in Outdated
Comment threadsrc/native/libs/System.Native/pal_uid.c Outdated
Comment threadsrc/native/libs/System.Native/pal_uid.c Outdated
Comment threadsrc/native/libs/configure.cmake Outdated

steveisok requested a review from akoeplinger May 18, 2022 01:35

stephentoub reviewed May 18, 2022
View reviewed changes

Comment threadsrc/native/libs/configure.cmake Outdated

jkotas mentioned this pull request May 18, 2022
[NativeAOT] System.Reflection.Tests timeout/hang #69476
Closed

adamsitnik reviewed May 18, 2022
View reviewed changes

Comment threadsrc/native/libs/configure.cmake Outdated

danmoseley reviewed May 18, 2022
View reviewed changes

Comment threadsrc/native/libs/System.Native/pal_uid.c Outdated

eerhardt reviewed May 18, 2022
View reviewed changes

Comment threadsrc/native/libs/System.Native/pal_uid.c Outdated

carlossanlop commented May 18, 2022
View reviewed changes

Comment threadsrc/native/libs/configure.cmake Outdated

carlossanlop commented May 18, 2022
View reviewed changes

Comment threadsrc/native/libs/System.Native/pal_uid.c Outdated

eerhardt previously approved these changes May 18, 2022
View reviewed changes

Comment threadsrc/native/libs/System.Native/pal_uid.c Outdated

tmds reviewed May 19, 2022
View reviewed changes

Comment threadsrc/native/libs/System.Native/pal_uid.c Outdated

akoeplinger reviewed May 19, 2022
View reviewed changes

Comment threadsrc/native/libs/System.Native/pal_uid.c Outdated

This comment was marked as outdated.
Sign in to view

Remove browser from src csproj
1340066

carlossanlop mentioned this pull request Jun 1, 2022
Move Format property to TarEntry and allow multiple Global Extended Attributes entries #69933
Closed
1 task

eerhardt reviewed Jun 1, 2022
View reviewed changes

Comment threadsrc/libraries/System.Formats.Tar/src/System.Formats.Tar.csproj Outdated

eerhardt reviewed Jun 1, 2022
View reviewed changes

Comment threadsrc/native/libs/System.Native/pal_uid.c Outdated

eerhardt approved these changes Jun 1, 2022
View reviewed changes

carlossanlop added 2 commits June 1, 2022 15:02

Join unix itemgroup with unix+browser itemgroup, remove browser mention
c71b04c

Check pthread_mutex_lock return value
3821728

eerhardt approved these changes Jun 1, 2022
View reviewed changes

carlossanlop merged commit 82a2562 into dotnet:mainJun 2, 2022

carlossanlop deleted the TarImprovements2 branch June 2, 2022 03:41

EgorBo mentioned this pull request Jun 7, 2022
Regressions in System.Formats.Tar.Tests.Perf_TarFile #70366
Closed

ghost locked as resolved and limited conversation to collaborators Jul 2, 2022

jeffhandley added area-System.Formats.Tar and removed area-System.IO labels Nov 21, 2022

Uh oh!

Conversation

carlossanlop commented May 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlossanlop commented May 18, 2022

Uh oh!

azure-pipelinesBot commented May 18, 2022

Uh oh!

carlossanlop commented May 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danmoseley commented May 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlossanlop commented May 18, 2022

Uh oh!

azure-pipelinesBot commented May 18, 2022

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlossanlop commented May 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlossanlop commented May 21, 2022

Uh oh!

azure-pipelinesBot commented May 21, 2022

Uh oh!

This comment was marked as outdated.

carlossanlop commented May 31, 2022

Uh oh!

azure-pipelinesBot commented May 31, 2022

Uh oh!

radical commented May 31, 2022

Uh oh!

carlossanlop commented May 31, 2022

Uh oh!

This comment was marked as outdated.

carlossanlop commented May 31, 2022

Uh oh!

eerhardt commented May 31, 2022

Uh oh!

carlossanlop commented May 31, 2022

Uh oh!

carlossanlop commented May 31, 2022

Uh oh!

azure-pipelinesBot commented May 31, 2022

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

carlossanlop commented May 18, 2022•
edited
Loading

carlossanlop commented May 18, 2022•
edited
Loading

carlossanlop commented May 19, 2022•
edited
Loading