Skip to content

[android] Improve CFKnownLocations. #2391

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

Merged
merged 1 commit into from
Jul 17, 2019

Conversation

drodriguez
Copy link
Contributor

The usage of CFKnownLocations from Swift Foundation seems to be reduced
to both _kCFKnownLocationUserAny and _kCFKnownLocationUserCurrent. From
the public API of Swift Foundation there's no way to get to
_kCFKnownLocationUserByName. Supporting UserAny is, therefore,
necessary.

There are some ways of accessing _kCFKnownLocationUserByName from the CF
API, but it seems that the API only allows Current or Any, even if they
are not enforced. In any case, Android do not have Unix usernames, so
ByName doesn't make sense, and should be always the same as UserCurrent.

The change in code makes every code path fall back to the current user
(which should be also any user, and any user by name, because the
platforms limitations).

Additionally, instead of picking up a CFFIXEDUSE_HOME, request the home
directory from CFUtilities, since it deals with HOME, HOMEDIR and
CFFIXED_HOME appropiedly, since HOME is necessary for many other things.

The current code will have aborted the test suite, while the modified
code, passes the test suite for UserDefaults.

@drodriguez drodriguez requested review from millenomi and compnerd July 1, 2019 22:05
break;
}
case _kCFKnownLocationUserAny:
case _kCFKnownLocationUserByName:
Copy link
Member

Choose a reason for hiding this comment

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

I think that this should still abort. CFCopyHomeDirectoryURL will give you the current user's home directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, read the explanation in the commit message. There's no users in Android. The sandboxing only allows the app to write to their sandbox. Any User, Current User, and User By Name are effectively the same. The app should set a HOME (or CFFIXED_USER_HOME) to point at some point in their sandbox. There's no going to be preferences for “any user” beside the current user, so trying to choose different directories is not worth it.

Besides, the abort in the previous code was making the test suite abort, because the test suite uses Any (through UserPreferences), and it is not possible to avoid so.

Copy link
Member

Choose a reason for hiding this comment

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

I did read the explanation, but that wasn't particularly clear from the commit message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to rewrite the commit message/PR description and resubmit? Or should I change something else?

We need at least Any and UserCurrent to work for the test suite to pass without Android-specific changes.

case _kCFKnownLocationUserByName:
// fallthrough
case _kCFKnownLocationUserCurrent: {
CFURLRef userdir = CFCopyHomeDirectoryURL();
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong for any user right? It is the current user's home directory.

@finagolfin
Copy link
Member

I can confirm this fixes the test abort and gets the suite to finish again when natively compiling Foundation on Android.

@millenomi
Copy link
Contributor

What is the test that is failing? In what way is this failing?

As I've noted several times: I'm not going to make an option mean something else without good reason. This applies to …Any and …UserByName as well.

@drodriguez
Copy link
Contributor Author

TestUserDefaults was the one aborting. I don’t remember the exact test, but I think it was almost all of them. UserDefaults uses the code from CFApplicationPreferences. That code registers several domains for the app, some with the app name, some with AnyApp; some with CurrentUser, and some with AnyUser; and some with CurrentHost and AnyHost. Since the AnyUser is a fallback of CurrentUser, if a query wasn't finding the key for the CurrentUser, it was looking into AnyUser, which was aborting trying to find the path. I don’t know if that's a perfect description, but it is close to what was happening.

About making options mean something different, the point that I'm trying to make is that in a system with no user accounts (like Android), the meaning of “any user” and “current user” do not make sense, and at the same time, they mean the same. It is a mono-user OS, so the “current user” is “The User”, and “any user” can only be “The User”. I can remove the code for “by name”, since I don’t think it is hit (at least by the test suite) but I extended my concept of mono-user to “every named user” should be “The User”.

@millenomi
Copy link
Contributor

We, however, inherit those concepts from Foundation needing to interop on all platforms. Note that iOS too has Any… and named users as API even if it's a single-user system.

Hmmm. Can we just fall back to the iOS behavior with the understanding that the home directory must be set with CFFIXED_USER_HOME?

@millenomi
Copy link
Contributor

(The home directory function already returns it.)

@drodriguez
Copy link
Contributor Author

I agree that we are not doing the same as iOS, which has been built with Foundation in mind, and have the right “holes” poked in the right places to allow sandboxed applications to work correctly with global preferences. However we don't have the same opportunity with Android. There's no global path for preferences in Android. The global preferences that one can read in Android have their physical representation hidden from apps, so there's no way we can simulate such thing (not even for read-only preferences, which I suppose are the only ones that work in iOS).

I completely understand the desire of making every enumeration to be the same meaning in every platform, but I also think that we should be pragmatic and adapt to each platform the best we can. Android doesn't have the concept of a global path for global preferences, but redirecting to an app local directory is the best we can do. Notice that even if iOS uses the concept of UserCurrent, the truth is that it is app local, not user local, like in macOS. One app cannot access the preferences of another app (and one need the concept of app containers to allow extensions to read the same preferences from even the same app).

About your question, the version I propose uses CFCopyHomeDirectoryURL which should do the right thing with CFFIXED_USER_HOME and HOME, while the existing version introduced its own code to deal with CFFIXED_USER_HOME, which I think duplicates efforts that should be de-duplicated. The only differences from the MACOS branch is that the Any case uses a Darwin-specific path, while Android tries to work around the fact that a global path will not be possible.

Personally I would prefer something that works, even if it doesn’t work the same as in other platforms, to something that doesn't work at all. Specially if the current implementation will simply abort for any usage of the UserDefaults.

@millenomi
Copy link
Contributor

iOS doesn't really open holes. (It's a lot more complex than that — but the only thing that needs it is reading specific systemwide preferences, which aren't a thing that happens on Android.)

The problem with this patch is as much as that we are conflating a platform concept with the availability of symbols, and also that we are making every CurrentUser domain overlap with every Any domain overlap with any named user domain.

Here's a proposal that mirrors the way ByHost preferences work on Darwin:

  • Any: Home + '/Apple/Library/Preferences/AnyUser'
  • Current: Home + '/Apple/Library/Preferences'
  • Named user: abort. (The issue is specifically with Any, right?)

@millenomi
Copy link
Contributor

(In general, I want to sequester stuff under $HOME/Apple so that we do not pollute paths that can be assumed free for use by third-party code. I think that we should do so in FileManager as well, making the user domain rooted at $HOME/Apple rather than $HOME.)

@millenomi
Copy link
Contributor

Proposal otherwise:

Named user: Home + '/Apple/Library/Preferences/ByUser/$USERNAME'

@drodriguez
Copy link
Contributor Author

I was about to change this to have support for Any and Current and remove support the named users. I think the named users are not really used in the code (I think it is impossible to hit those code paths from Foundation).

I will explore the usage of different folders for Any and Current (and for the named users).

The usage of CFKnownLocations from Swift Foundation seems to be reduced
to both _kCFKnownLocationUserAny and _kCFKnownLocationUserCurrent. From
the public API of Swift Foundation there's no way to get to
_kCFKnownLocationUserByName. Supporting UserAny is, therefore,
necessary.

There are some ways of accessing _kCFKnownLocationUserByName from the CF
API, but it seems that the API only allows Current or Any, even if they
are not enforced. In any case, Android do not have Unix usernames, so
ByName doesn't make sense.

The change maps AnyUser into a directory inside the preferences of the
HOME of the user, and other user names into another subdirectory. Each
app will have their own set of Any and ByName, but at least they are not
confussed with the CurrentUser preferences.

Additionally, instead of picking up a CFFIXEDUSE_HOME, request the home
directory from CFUtilities, since it deals with HOME, HOMEDIR and
CFFIXED_HOME appropiedly, since HOME is necessary for many other things.

The current code will have aborted the test suite, while the modified
code, passes the test suite for UserDefaults.
@drodriguez drodriguez force-pushed the android-known-locations branch from de6482e to ab5cb5a Compare July 16, 2019 22:19
@drodriguez
Copy link
Contributor Author

Changed the paths to include the /Apple prefix and the AnyUser and ByUser for the cases they make sense. TestUserDefaults pass on Android and doesn't abort.

@swift-ci please test Linux platform

@drodriguez
Copy link
Contributor Author

Failures in LLDB are unrelated.

@swift-ci please test Linux platform

@millenomi
Copy link
Contributor

Thank you!

@millenomi millenomi merged commit 7a3cf7f into swiftlang:master Jul 17, 2019
@drodriguez drodriguez deleted the android-known-locations branch July 22, 2019 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants