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

[tests] Disables tests on Apple mobile platforms #95757

Merged
merged 6 commits into from
Dec 11, 2023

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Dec 7, 2023

Description

This PR disables tests on Apple mobile platforms due to temporary directory that exceeds the path length limit.

@ghost
Copy link

ghost commented Dec 7, 2023

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

The folder used to store files needs to have a shorter path on Apple mobile platforms as it exceeds 100 bytes (/Users/helix-runner/Library/Developer/CoreSimulator/Devices/XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX)

Fixes #88049

Author: kotlarmilos
Assignees: kotlarmilos
Labels:

os-ios, area-System.Formats.Tar

Milestone: 9.0.0

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

#if TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS
// the folder used to store files needs to have a shorter path on Apple mobile platforms,
// because the TempDirectory gets created in folder with a path longer than 100 bytes
using TempDirectory root = new TempDirectory("/Users/helix-runner/Library/Developer/CoreSimulator/Devices/tempDir");
Copy link
Member

Choose a reason for hiding this comment

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

This should work on simulator and maybe catalyst, but should fail on device. As far as I know, there's no way to shrink the app bundle path (where your app gets a tmp directory).

Copy link
Member Author

Choose a reason for hiding this comment

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

According to the TempDirectory class, it is possible to specify the full path:

public TempDirectory(string path)
{
    Path = path;
    Directory.CreateDirectory(path);
}

One concern regarding this is permissions and possibility to create a particular directory.

Copy link
Member Author

@kotlarmilos kotlarmilos Dec 8, 2023

Choose a reason for hiding this comment

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

It seems that the #if TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS condition in the library test is evaluated as FALSE on Apple mobile targets. Without the macro condition and with shorter temporary directory using TempDirectory root = new TempDirectory("/Users/helix-runner/tmp"); the tests seem to work on Apple mobile targets.

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 making this change! Left some comments for you to consider.

#if TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS
// the folder used to store files needs to have a shorter path on Apple mobile platforms,
// because the TempDirectory gets created in folder with a path longer than 100 bytes
using TempDirectory root = new TempDirectory("/Users/helix-runner/Library/Developer/CoreSimulator/Devices/tempDir");
Copy link
Member

Choose a reason for hiding this comment

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

Can you please store the string "/Users/helix-runner/Library/Developer/CoreSimulator/Devices/tempDir" in a field, to avoid repetition?

#if TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS
// the folder used to store files needs to have a shorter path on Apple mobile platforms,
// because the TempDirectory gets created in folder with a path longer than 100 bytes
using TempDirectory root = new TempDirectory("/Users/helix-runner/Library/Developer/CoreSimulator/Devices/tempDir");
Copy link
Member

Choose a reason for hiding this comment

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

The /Users/helix-runner/Library/Developer/CoreSimulator/Devices/tempDir string is 68 chars long. In the tests where we use them, feel free to reduce the names of the files to even shorter lengths if needed (let's see if the CI does not complain). For example, instead of file.txt, you can just use f.

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 try to shrink the path even more. One concern regarding this is permissions and possibility to create a particular directory.

#if TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS
// the folder used to store files needs to have a shorter path on Apple mobile platforms,
// because the TempDirectory gets created in folder with a path longer than 100 bytes
using TempDirectory root = new TempDirectory("/Users/helix-runner/Library/Developer/CoreSimulator/Devices/tempDir");
Copy link
Member

Choose a reason for hiding this comment

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

Do you have control over tempDir? Could it just be tmp, or t?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is an arbitrary name and probably we can set even shorter name.

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslikesimulator

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslikesimulator

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@akoeplinger
Copy link
Member

I don't think we should make this change, as Steve said this will never work on devices and even for simulators writing outside of the simulator-managed paths is a big no no (paths won't get cleaned up when the app is uninstalled etc).

I think we should just disable these tests.

@kotlarmilos
Copy link
Member Author

The tests are already disabled. We can close this PR.

@akoeplinger
Copy link
Member

@kotlarmilos can you please replace the ActiveIssue attributes with SkipOnPlatform and add a comment?

@kotlarmilos kotlarmilos reopened this Dec 11, 2023
@kotlarmilos kotlarmilos changed the title [tests] Use shorter path for temp directory on Apple mobile platforms [tests] Disables tests on Apple mobile platforms Dec 11, 2023
@kotlarmilos
Copy link
Member Author

The tests are failing on the release/net7.0. Should we backport this PR and then close the tracking issue?

@kotlarmilos kotlarmilos marked this pull request as ready for review December 11, 2023 11:29
@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@akoeplinger
Copy link
Member

The tests are failing on the release/net7.0. Should we backport this PR and then close the tracking issue?

Yes.

@kotlarmilos kotlarmilos merged commit e42c683 into dotnet:main Dec 11, 2023
117 of 127 checks passed
@kotlarmilos
Copy link
Member Author

/backport to release/7.0-staging

Copy link
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/7169718852

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants