Implementation of XDG File specification and a basic implementation of HTTPCookieStorage #889

mamabusi · 2017-02-22T11:01:18Z

This is an unpolished implementation of the XDG File specification. Have referred to related work started by Tony Parker in the repo: https://github.com/parkera/swift-corelibs-foundation/tree/parkera/xdg_implementation

Note that the tests for the API implementation have been done on my local build setup. The same are not available in the committed changes here as they involve adding API in the Foundation to access the CoreFoundation implementation that is done.
Please suggest if there is an alternate way of adding tests.

parkera · 2017-02-22T19:12:43Z

The testing is absolutely the hardest part of this; we also need a way to start new child processes with new environment variables to make sure everything is configured correctly.

mamabusi · 2017-02-23T09:33:28Z

I have written to the community for ideas to get the tests done. Only way I could think of now is to use the existing APIs (one example being HTTPCookieStorage API) that use these environmental variables to get a part of the tests done. Need to check on where the remaining environmental variable usage would fit in.

Regarding the "start new child processes with new environment variables to make sure everything is configured correctly", does this mean that we would be setting the environmental variables using these child processes and the test API is expected to reflect the values set?
I am looking into the 'Process' API that would help in creating the child process and launch the tasks. Would that be a right approach?
Thanks!

parkera · 2017-02-24T19:05:15Z

Yes, probably we'll need to create a helper process that gets launched via Process with the specific environment variables set. Then we need that process to use some existing API that would be affected by the presence of the environment variables (preferences, maybe?). Probably then it would exit with a success or failure code, which the test harness would then interpret as either success or failure.

mamabusi · 2017-03-01T10:25:18Z

@parkera Ok, will get onto it. Meanwhile, request you to review the implementation thus far.
Thanks!

parkera · 2017-03-01T18:34:29Z

Yup, looks reasonable so far. Thanks!

mamabusi · 2017-03-07T10:33:47Z

@parkera By mentioning "Then we need that process to use some existing API that would be affected by the presence of the environment variables (preferences, maybe?)", did you mean to use the NSUserDefaults on Foundation?

parkera · 2017-03-07T16:56:23Z

Yes, that would be a good candidate. NSUserDefaults has a search list that looks at several locations in a particular order to find a value for a user default. On Darwin this includes places like ~/Library, /Library, etc. With this XDG stuff implemented, I would expect it to affect the places that CFPreferences (which is behind NSUserDefaults) looks for those values on Linux.

mamabusi · 2017-03-07T18:57:07Z

@parkera At the outlook, the NSUserDefaults code on Linux currently does not look complete. I see api code in it being commented stating compiler failures / code confined to only OS x/ iOS platforms / unimplemented api and also the test-cases on TestFoundation are commented out.
There is this init that was of interest to me as it mentions about the search list:

/// nil suite means use the default search list that +standardUserDefaults uses
    public init?(suiteName suitename: String?) {
        suite = suitename
    }

On a search done in Foundation for 'standardUserDefaults' , the results do not yield the setting or use of this elsewhere. This implies that there is no default value set on Linux?
Any comments on this?
Sorry if I am totally off-beat here. I would look into further (on the CFPreferences as well) to check if I can find something.
Thanks!

e78l · 2017-03-07T22:55:44Z

Hello @mamabusi
Hoping this info could help:
standardUserDefaults was changed to standard in UserDefaults.swift

I found this to be a good read about suite names and such:
http://dscoder.com/defaults.html

CFXMLPreferencesDomain.c may be the simplest way to get things working; it seems to have been checked in[1], but some other changes in CF are needed[2]
[1] https://github.com/apple/swift-corelibs-foundation/blob/master/CoreFoundation/Preferences.subproj/CFXMLPreferencesDomain.c
[2] Delete CFPreferences.c line 500 + change the URL paths, perhaps in _CFPreferencesURLForStandardDomainWithSafetyLevel
https://github.com/apple/swift-corelibs-foundation/blob/master/CoreFoundation/Preferences.subproj/CFPreferences.c#L500

TestNSUserDefaults.swift has test_getRegisteredDefaultItem but that only works since there's a volatile domain/dictionary in UserDefaults.swift. More tests are probably needed.

@parkera
I've been thinking of some arguments to switch from XML settings to JSON(/binary PLIST/YAML?) on linux but don't have anything formal. My main arguments for the switch would be domains+hosts aren't necessary/common enough for Linux, and inter-OS file compatibility isn't a significant factor (few/no one would copy settings across platforms).
This might also be a good opportunity to rewrite UserDefaults.swift, perhaps with cleaner file writing/locking. I know there're reasons for not re-writing SCL in Swift, but I perceive the overhead of CF user defaults to be large.
Is this something worth discussing/pursuing?

parkera · 2017-03-09T01:19:22Z

User defaults are simply property lists (which can be XML or binary), which is basically the lingua franca of CoreFoundation. I don't think there is much value in changing the format; but if we did it would be orthogonal to getting the basic API working. I do want to keep the CFPrefs concept of search lists in place, although some (like the network home) will not make sense on this platform.

Maybe instead of user defaults, which has its own set of challenges here, we should focus on the use case which got us here in the first place? I think that was where URLSession should store its cached data.

mamabusi · 2017-03-10T09:07:00Z

@e78l @parkera Thanks for your time and info provided here.

@parkera It was NSHTTPCookieStorage that got us to XDG implementation and as suggested I plan to use the same for adding in test-cases.

pushkarnk · 2017-03-10T09:16:29Z

@mamabusi I will update the HTTPCookieStorage PR by early next week, its been a while

pushkarnk · 2017-03-14T06:56:05Z

@mamabusi @parkera For #672 I plan to use XDG_CONFIG_HOME. Though XDG_DATA_HOME sounds better for cookies, XDG_CONFIG_HOME seems to be commonly used. We can change it later if needed.

e78l · 2017-03-16T01:01:57Z

@parkera OK, and thanks for the response. There do seem to be some file size+performance penalties from writing a plist to XML (repeatedly), but as I also like readable settings, there isn't much to change (XML vs binary plist).
@mamabusi No problem!

mamabusi · 2017-03-16T07:12:19Z

@parkera Could the commit of HTTPCookieStorage be combined with this?
The reason being that it uses XDG_CONFIG_HOME and has its test-suite in place.
Thanks!

pushkarnk · 2017-03-16T07:21:59Z

@mamabusi For #672 I am in the process of wrapping up with the tests. Though an initial implementation is in place, I still need to tweak it to use the XDG_CONFIG_HOME.

mamabusi · 2017-03-16T09:05:45Z

@pushkarnk Cool. Once the changes are in place for using the XDG variable, I'll verify the correctness of the path retrieval in HTTPCookieStorage. Thanks!

mamabusi · 2017-03-17T12:29:26Z

@parkera
Hi
Regarding the setting of the XDG environment variables, I had a couple of queries.

I set the environment variable using the Foundation Api:
setenv("XDG_CONFIG_HOME", "/usr/test", 1)

Retrieved it using the api:
let rawValue = getenv("XDG_CONFIG_HOME")
let sampleString = String(utf8String: rawValue!)
print(sampleString!)

I could fetch the variable that is set.

Whereas, on the CoreFoundation when I try to fetch this using the API __CFgetenv(), I see that the variable is not set.

Wondering what is the reason that it is not reflecting on the CoreFoundation side. Do they refer to different scopes of environment? Aren’t all of these running in the same process?

Also, if I have to set the system-wide environment variables (in this case the XDG variables should be system-wide) after much reading I figured out that it is not possible programmatically. Hence, I would have to do it by launching a script file that would edit the system (read-only) files that require the root access of the machine where the tests are to be run. Is this a feasible option? Or is there a alternative that is already being used in TestFoundation? Am I missing something here?
Your inputs would be helpful.

Thanks!

parkera · 2017-03-17T16:23:40Z

__CFgetenv caches the result of getenv for performance reasons; so you will have to make sure the environment variable is set before the process runs.

mamabusi · 2017-04-04T06:17:51Z

lib/phases.py

@@ -417,6 +417,7 @@ class SwiftExecutable(BuildPhase):
    def __init__(self, executableName, sources):
        BuildAction.__init__(self, output=executableName)
        self.executableName = executableName
+        self.name = executableName


Adding a dependency of the executable creation for XDG to foundation_tests (which is the SwiftExecutable: Test Foundation) in build.py caused a script failure here as the 'name' seemed to be 'nil'. Hence, the change.

mamabusi · 2017-04-04T06:21:43Z

@pushkarnk Have cherry picked your HTTPCookieStorage implementation and modified the same to use the environment variable of XDG file specification. Please review.

@parkera Have added test-case for XDG file specification implementation. Please review.

Thanks!

pushkarnk · 2017-04-04T06:32:45Z

@swift-ci please test

pushkarnk · 2017-04-04T06:44:40Z

@swift-ci please test

pushkarnk · 2017-04-04T06:45:07Z

Third time again!
@swift-ci please test

pushkarnk · 2017-04-04T07:13:54Z

Looks like a swiftpm failure:

/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swiftpm/Tests/SourceControlTests/RepositoryManagerTests.swift:188: error: RepositoryManagerTests.testReset : XCTAssertEqual failed: ("1") is not equal to ("2") -

mamabusi · 2017-04-20T21:03:18Z

CoreFoundation/Base.subproj/CFFileUtilities.c

+    }
+    // Add count for trailing path unless ending with colon.
+    pathCount += last_colon < (values + strlen(values) - 1);
+    if (pathCount > 64) {


Limits the number of paths to 64 and returns an empty array if pathCount exceeds 64.

mamabusi · 2017-04-20T21:07:35Z

CoreFoundation/Base.subproj/CFFileUtilities.c

+    CFArrayRef dataDirPathsArray = _CFCreateCFArrayByTokenizingString(dataDirectoriesPaths, ':');
+    if(CFArrayGetCount(dataDirPathsArray) == 0) {
+        CFStringRef logMessage = CFSTR("Value set in XDG_DATA_DIRS variable not honoured. Returning the default.");
+        CFLog(kCFLogLevelWarning, CFSTR("%@"), logMessage);


In the case of the pathCount > 64 or all the paths in the variable turn out to be invalid, an empty array is returned. At such times the default path is returned although the variable was set. The log warning message gives a hint.

mamabusi · 2017-04-20T21:08:00Z

CoreFoundation/Base.subproj/CFFileUtilities.c

+    CFArrayRef configDirPathsArray = _CFCreateCFArrayByTokenizingString(configDirectoriesPaths, ':');
+    if(CFArrayGetCount(configDirPathsArray) == 0) {
+        CFStringRef logMessage = CFSTR("Value set in XDG_CONFIG_DIRS variable not honoured. Returning the default.");
+        CFLog(kCFLogLevelWarning, CFSTR("%@"), logMessage);


In the case of the pathCount > 64 or all the paths in the variable turn out to be invalid, an empty array is returned. At such times the default path is returned although the variable was set. The log warning message gives a hint.

Ok; one more thing now - we need to release configDirPathsArray here before returning, otherwise we leak it.

@parkera Sorry I missed that. Done now.

mamabusi · 2017-04-20T21:10:17Z

@parkera Addressed the requested changes. Please take a look.

alblue · 2017-04-21T13:33:33Z

@swift-ci please test

pushkarnk · 2017-04-24T09:43:54Z

Looks like the tests didn't run

@swift-ci please test

mamabusi · 2017-04-24T17:04:27Z

@parkera All the review changes are completed. Please let me know if something is missed.
Thanks!

parkera · 2017-04-24T17:31:12Z

Thanks @mamabusi! Let's get this merged.

mamabusi · 2017-04-24T17:37:33Z

Yay! Thanks a ton for all the guidance @parkera

parkera · 2017-04-24T23:40:37Z

No problem, thank you for finishing this up.

ahti · 2017-05-05T20:18:07Z

I know this has been merged already, but I don't think the current paths for cookie storage are appropriate.

Firstly, $XDG_CONFIG_HOME (.config by default) is supposed to be used for "user specific configuration files". I don't think cookies fit that description. $XDG_DATA_HOME (.local/share by default), used for "user specific data files", sounds like a better fit to me.

Secondly, none of my Linux machines (Arch Linux and Ubuntu) contain any files in either .config or .local/share. Every application, even ones with only one file, create their own subfolder. The specification is not clear on this matter, but it does state:

Other specifications may reference this specification by specifying the location of a data file as $XDG_DATA_DIRS/subdir/filename.

This to me seems like reason enough to put any files Swift creates in a dedicated subfolder as well. This subfolder should probably be a separate folder per application, but see the following paragraphs for that.

Finally, the current implementation uses the same shared storage for all users of the api, while that behavior was removed from OSX Foundation. The doc comment of HTTPCookieStorage.shared says:

Starting in OS X 10.11, each app has its own sharedHTTPCookieStorage singleton, which will not be shared with other applications.

I think it would be reasonable for the Linux implementation to have the same behaviour, although that change might be better to hold back until corelibs-foundation has a more fleshed-out concept of different directories on Linux including implementing FileManager.url(for: ...), probably returning per-application subdirectories of xdg directories.

parkera · 2017-05-05T22:46:10Z

Those comments do make sense to me. It would be good to get them into a bug on bugs.swift.org, and perhaps follow up with a discussion on swift-corelibs-dev mailing list.

ianpartridge · 2017-05-06T08:21:36Z

@mamabusi can you take a look at this?

mamabusi · 2017-05-08T11:45:22Z

@ahti Thanks for the suggestions.
The choice of XDG_DATA_HOME and XDG_CONFIG_HOME for cookie storage did come up during the implementation. We (@pushkarnk and me) had a discussion on the mailing list and based on the inputs we got, went with the XDG_CONFIG_HOME. As the XDG_DATA_HOME seems a better fit here, I will make the necessary changes and we could discuss on the mailing list for the consensus.

Regarding "Starting in OS X 10.11, each app has its own sharedHTTPCookieStorage singleton, which will not be shared with other applications" I will modify the current implementation on Linux to match that of Darwin.

Thanks!

ahti · 2017-05-12T02:19:13Z

Sorry, I've been quite busy the past week.

@mamabusi Sounds good. Should I still file a bug in jira?

mamabusi · 2017-05-15T06:48:32Z

@ahti Yes, please go ahead and file a bug for this in JIRA. Thanks!

ahti · 2017-05-16T13:45:55Z

I've split up my comment and filed two bugs.

pushkarnk mentioned this pull request Mar 6, 2017

Initial implementation of HTTPCookieStorage #672

Closed

2 tasks

mamabusi force-pushed the xdg-impl-branch branch from f4c3182 to c013c5c Compare March 21, 2017 08:59

mamabusi force-pushed the xdg-impl-branch branch 3 times, most recently from fe63707 to 0c05efd Compare April 4, 2017 06:12

mamabusi commented Apr 4, 2017

View reviewed changes

mamabusi force-pushed the xdg-impl-branch branch from a2c0308 to 78d865e Compare April 20, 2017 20:54

mamabusi and others added 4 commits April 21, 2017 02:27

Implementation of XDG File specification.

741af72

Initial implementation of HTTPCookieStorage

add28ba

Included testcase for the implementation of XDG specification.

24e8ad5

Added the file required to create the new executable.

c493b04

mamabusi force-pushed the xdg-impl-branch branch from 78d865e to 168402f Compare April 20, 2017 20:58

mamabusi commented Apr 20, 2017

View reviewed changes

Modified code changes to address the review comments.

8451b05

mamabusi force-pushed the xdg-impl-branch branch from 168402f to 8451b05 Compare April 21, 2017 05:55

parkera approved these changes Apr 24, 2017

View reviewed changes

parkera merged commit e4cfd27 into swiftlang:master Apr 24, 2017

ianpartridge mentioned this pull request Apr 25, 2017

[3.1] Backport HTTPCookieStorage #963

Merged

ahti mentioned this pull request May 5, 2022

[SR-4905] Store HTTPCookieStorage data in XDG_DATA_HOME #3858

Open

SDGGiesbrecht mentioned this pull request Aug 28, 2023

~/.swiftpm config, cache to follow the XDG base dir spec swiftlang/swift-package-manager#6204

Closed

Implementation of XDG File specification and a basic implementation of HTTPCookieStorage #889

Implementation of XDG File specification and a basic implementation of HTTPCookieStorage #889

Uh oh!

Conversation

mamabusi commented Feb 22, 2017

Uh oh!

parkera commented Feb 22, 2017

Uh oh!

mamabusi commented Feb 23, 2017

Uh oh!

parkera commented Feb 24, 2017

Uh oh!

mamabusi commented Mar 1, 2017

Uh oh!

parkera commented Mar 1, 2017

Uh oh!

mamabusi commented Mar 7, 2017

Uh oh!

parkera commented Mar 7, 2017

Uh oh!

mamabusi commented Mar 7, 2017

Uh oh!

e78l commented Mar 7, 2017

Uh oh!

parkera commented Mar 9, 2017

Uh oh!

mamabusi commented Mar 10, 2017

Uh oh!

pushkarnk commented Mar 10, 2017

Uh oh!

pushkarnk commented Mar 14, 2017

Uh oh!

e78l commented Mar 16, 2017

Uh oh!

mamabusi commented Mar 16, 2017

Uh oh!

pushkarnk commented Mar 16, 2017

Uh oh!

mamabusi commented Mar 16, 2017

Uh oh!

mamabusi commented Mar 17, 2017

Uh oh!

parkera commented Mar 17, 2017

Uh oh!

mamabusi Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

mamabusi commented Apr 4, 2017

Uh oh!

pushkarnk commented Apr 4, 2017

Uh oh!

pushkarnk commented Apr 4, 2017

Uh oh!

pushkarnk commented Apr 4, 2017

Uh oh!

pushkarnk commented Apr 4, 2017

Uh oh!

mamabusi Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

mamabusi Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

mamabusi Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

parkera Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

mamabusi Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

mamabusi commented Apr 20, 2017

Uh oh!

alblue commented Apr 21, 2017

Uh oh!

pushkarnk commented Apr 24, 2017

Uh oh!

mamabusi commented Apr 24, 2017

Uh oh!

mamabusi commented May 8, 2017 •

edited

Loading