Plugin wipes out cordova-plugin-add-swift-support configuration

[djett41]

The cordova-plugin-add-swift-support allows us to use legacy swift version 2.3, as well as consolidates Bridging-Header files from plugins into the projects Bridging-Header.h.

Its hook will add the following to the project.pbxproj for debug and release XCBuildConfiguration buildSettings when adding support for legacy swift.

SWIFT_OBJC_BRIDGING_HEADER = "$(PROJECT_DIR)/$(PROJECT_NAME)/Bridging-Header.h";
ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES = YES;
SWIFT_VERSION = 2.3;
SWIFT_OPTIMIZATION_LEVEL = "-Onone";

This plugin will wipe out those values, when it should merge with them and overwrit if present, but not wipe out vlaues that arent defined for this plugin

[dpa99c]

I am unable to produce this issue with a simple test case - see this screencast.

Can you provide a reproducible scenario in which this plugin will implicitly "wipe out" the configuration added by cordova-plugin-add-swift-support?

Unlike other plugins which have hard-coded custom configuration keys built into them (e.g. cordova-universal-links-plugin), this plugin is simply a hook script which maps configuration placed in the config.xml to the relevant native config file (in this case project.pbxproj) so unless you explicitly add entries with the same key names as you quote above to the config.xml (e.g. <preference name="ios-XCBuildConfiguration-SWIFT_OBJC_BRIDGING_HEADER" value="foo" />) then any existing configuration keys, be they added by another plugin or otherwise, will not be removed or overwritten.

The intention of this plugin is that it allows advanced custom configuration of the native project files and hence its use is with the caveat that the user understands what they are modifying.
Hence, this plugin (at least currently) does not make any attempt to merge existing values of XCBuildConfiguration block keys into those being injected: as documented, the existing value will be overwritten:

If an entry already exists in an XCBuildConfiguration block for the specified key, the existing value will be overwritten with the specified value.

So, for example, if I were using cordova-plugin-add-swift-support in a project that used this plugin to modify a configuration key used by the former, it would be prudent of me to manually merge the configuration I expected from the swift plugin into my custom configuration block, so when the key is overwritten, the original value is not lost, for example:

<preference name="ios-XCBuildConfiguration-SWIFT_OBJC_BRIDGING_HEADER" value="$(PROJECT_DIR)/$(PROJECT_NAME)/Bridging-Header.h;foo" quote="value" />

[djett41]

Ahh meant to post more info when I got it, the issue was that there was a preference in the config.xml called

<preference name="ios-statusbarstyle" value="black-opaque"/>

but the plugin hook saw this as a XCBuildConfiguration property and modified the buildSettings values, wiping out any modifications from the swift plugin. If you add that pref with the swift plugin you should be able to see what im talking about when doing a platform rm, platform add, and prepare

Since you cant know exactly how the Cordova CLI, or any other plugins or plugin hooks can modify project files now and in the future, the best thing would be to merge any of your custom config modifications with whats already in the value for buildSettings... so if I have SOME_PROP = "value" in the XCBuildConfiguration buildSettings, then that should also be present in the buildSettings after this plugins hook runs even if I have custom config from this plugin, or if some other plugin uses the prefix ios- like in my example above.

so in a nutshell...the hook is causing to wipe out other buildSettings values from the swift plugin, which is definitely something that should be addressed.

Thanks!

[dpa99c]

the plugin hook saw this as a XCBuildConfiguration property and modified the buildSettings values, wiping out any modifications from the swift plugin

I'm unable to reproduce the issue by adding the preference <preference name="ios-statusbarstyle" value="black-opaque"/>.

However I can see that it would match the iOS preference regex, causing the plugin to read then write the project.pbxproj file. However the plugin won't see this preference as an XCBuildConfiguration property because it does not contain that prefix. Instead it will assign an internal type of statusbarstyle which would not match any cases in the switch statement which processes the config items.

The fact that this preference would cause the custom config plugin to read then write the project.pbxproj without any changes to the content is an indicator to me of what I think is happening: I think a race condition is possible where the two plugins are concurrently accessing the project.pbxproj file. This would explain why the config added by the swift plugin is being lost: the custom config plugin is reading the file contents before the swift plugin has written its changes back to disk hence when the custom config plugin writes its changes, the swift plugin changes are not present.

This plugin returns a promise in order to block further execution in the Cordova hooks lifecycle until it has finished its operations. The swift plugin does not, so I think it's possible for the custom config plugin to be invoked before the swift plugin has written back its changes.

I will attempt to reproduce this scenario and confirm whether this is the case.
And if so, the whole thing about merging config changes is irrelevant since this is an issue with concurrent file access.

Since you cant know exactly how the Cordova CLI, or any other plugins or plugin hooks can modify project files now and in the future, the best thing would be to merge any of your custom config modifications with whats already in the value for buildSettings... so if I have SOME_PROP = "value" in the XCBuildConfiguration buildSettings, then that should also be present in the buildSettings after this plugins hook runs even if I have custom config from this plugin, or if some other plugin uses the prefix ios- like in my example above.

This is simple to say, but not to implement, since merging existing values requires knowledge of the given key's value type, because depending on the field type, XCode uses different values or separators. And therefore is unlikely to be implemented any time soon.

[djett41]

got ya, I think you are right about the concurrent file access. I was about to say when I originally wrote this hook, I never had any concurrent modification issues, however that was back before Cordova even allowed you to execute hooks async.. That makes me think that plugins that modify some of the main Cordova project files, such as Manifest, plist, or any Xcode project files, should prob do so synchronously to ensure there are no race conditions.

That wouldnt really change much from an overall time perspective since the plugin reads all the files sync.

[dpa99c]

yep, of course it could be this hook script that's causing the concurrency issue, but I'm reckoning, whatever the cause, that's looking like the issue here.

I kind of hope it is this plugin/hook script causing the problem, cause then I can fix it :-) If it's internal to how Cordova is executing its hooks, that's more tricky...

[dpa99c]

OK, leave this with me. It certainly warrants some looking at how this hook script is interacting with the filesystem. Will keep you posted with what I find.

BTW sorry if I sound bitchy in comments above - been a long day :-)

[djett41]

cool no worries! sounds good.

[dpa99c]

OK, I think I've found and fixed the problem. Parsing the project.pbxproj file using the xcode module is async, therefore the promise was being resolved before the plugin had written the file to disk, creating the race condition we speculated existed.
By waiting for this async operation to complete, it forces the plugin not to resolve its promise too early, and should therefore fixe the problem.

I've pushed this fix out in a patch release: cordova-custom-config@3.1.3, so please give it a try and confirm that it resolves the issue for you.

[djett41]

awesome! I'll be able to confirm a little later (have to switch to a different laptop where the code is) but sounds good. Thanks!

[dpa99c]

Pretty sure this issue is now fixed. Let me know if not and I'll reopen.

[djett41]

Ahhh my bad forgot all about this. Sounds good thanks again!!