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

Auto-run hook for Xcode project settings modifications #154

Closed
wants to merge 4 commits into from

Conversation

derwaldgeist
Copy link
Contributor

Hi @ibc,

this is to automize the integration of the Xcode project settings hook into the build process, so the users of the plugin won't have to integrate the hook themselves. I made this because the standard way of integrating hooks is not possible with Meteor.

Some implementation notes:

  • The hook now runs "after_prepare", because otherwise Meteor would reset the Xcode project settings once the prepare phase is run. I don't know why exactly, but I found this out watching the Xcode project file during the build process. Details can be found here: https://forums.meteor.com/t/cordova-how-to-apply-platform-hook-with-meteor/17692
  • I've replaced the require() for xcode by context.requireCordovaModule('xcode'), because otherwise the script won't find this package when run in the Meteor context.
  • xcodeProject.parseSync() instead of parse() reduces the risk that two plugin hooks run in parallel would override their modifications. This was inspired by a comment in the cordova-disable-bitcode plugin.
  • I changed the minimum iOS version to 9.0, as you are supporting this according to your docs and I did not see any reason why to limit this further.
  • I have only tested the plugin in Meteor, not in plain vanilla Cordova, but it should work there, too.
  • I've also added .editorconfig and .jsbeautifyrc for convenience. Otherwise, my Atom editor would have changed the existing coding style, and maybe this will help other contributors, too.

There is also a patch to the FakeWebSocket implementation. The existing code would break Meteor's web sockets in rare cases, when close() was called before this.ws was available. Maybe that's a side-effect of setting this.ws inside a timeout(). Just checking this.ws and returning if it is not available yet is of course not optimal, since this will not close the native websocket. Maybe you'll find a better way to handle this.

@ibc
Copy link
Collaborator

ibc commented Apr 14, 2016

I need to check this change in vanilla Cordova before merging it.

Regarding FakeWebSocket I'm about removing it from the plugin sources and, instead, creating an ugly NPM package called websocket-hack-for-ios.

@ibc ibc mentioned this pull request Apr 26, 2016
@xpluscal
Copy link

@derwaldgeist sorry to bother you, but do you know how i can implement your pull request in meteor?

@xpluscal
Copy link

I managed to add the plugin, but get a series of errors from xcode, mostly related to WebAppLocalServer.swift and AssetBundleDownload.swift complaining about undeclared types.
@derwaldgeist did you have the same experience?

@xpluscal
Copy link

I fixed the issue.
Since you overwrote the pre-existing Bridging Header, a line of code got lost, which was implementing the cordova-plugin-meteor-webapp.

#import <Cordova/CDV.h>
#import "cordova-plugin-meteor-webapp-Bridging-Header.h"

adding those lines to the new Bridging Header fixes the issue.
This is just a temporary fix, but I guess there is a more sophisticated solution to the problem.

@ibc
Copy link
Collaborator

ibc commented Apr 26, 2016

@derwaldgeist @xpluscal I want to merge this PR, but I cannot spend time checking it (other than testing that it works with vanilla cordova). Can you please recheck it and commit whatever is missing please?

insert_final_newline = true

[*.md]
trim_trailing_whitespace = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you make these a separate commit? they have nothing to do with the commit they are currently bundled with

Copy link
Contributor Author

@derwaldgeist derwaldgeist Apr 26, 2016

Choose a reason for hiding this comment

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

Hm, TBH, I am not a Github expert (my private projects are using SVN), and I don't know how to take a commit out of an existing PR. If you don't like the additional .editorconfig, I recommend that you just delete it afterwards. It was just for the sake of convenience, to make life easier for upcoming contributors.

@derwaldgeist
Copy link
Contributor Author

Hi @xpluscal

Since you overwrote the pre-existing Bridging Header

This was part of the original code of the eface2face plugin setup script, which I did not change. I suppose you tried it in Meteor 1.3. This Meteor version uses Swift as well, hence the conflict. I am still on Meteor 1.2, so I did not notice this. Did you modify the script so it patches the bridging header instead of overwriting it, if it is already available? I think that's generally a good idea. This whole briding header thing is sometwhat stupid, I wonder why Apple wasn't able to solve this in a more elegant way.

And thanks for catching this.

@nunovieira
Copy link

@derwaldgeist are you still using Meteor 1.2?

I'm at Meteor 1.3 and looking for a solution to this problem.
If I understand correctly, iosrtc-swift-support.js sets the Objective-C Bridging Header to cordova-plugin-iosrtc-Bridging-Header.h, which I guess doesn't play well with other plugins.
I think the best approach would be like iosAddBridgingHeader.js in cordova-plugin-meteor-webapp. Any thoughts?

@derwaldgeist
Copy link
Contributor Author

Yes, I am still on Meteor 1.2. Did not dare to do the update. Last time (1.1 => 1.2) it took me months to adapt my app.

@saghul
Copy link
Collaborator

saghul commented Nov 14, 2016

@ibc I'm a bit on the fence with this one. At the moment I run a modified hook script (for exampkle, to set the development team) and I expect others to do the same TBH. So running it could be wasteful at the very least, and harmful in the worst case... WDYT?

@saghul
Copy link
Collaborator

saghul commented Nov 21, 2016

I committed the editorconfig change in 7e093b0, but I'm rejecting the rest of the PR based on my reasoning here: #154 (comment)

@saghul saghul closed this Nov 21, 2016
@derwaldgeist
Copy link
Contributor Author

@saghul I saw it from the perspective of a user who is not familiar with the internals of Cordova plugins and wants to have a plug-and-play integration. Having to modify Xcode settings manually is tedious as you have to do it for every fresh build, and it establishes a substantial hurdle for people who just want to use your open-source app instead of developing it. Writting your own custom scripts requires some in-depth knowledge about how Cordova hooks work. In my case, I am also using a script for setting the dev team, but I am doing it in a separate plugin dedicated to this configuration setting.

I would say, it would be best if the plugins adjust all configuration settings that are actually required to make them work, but they should also document these modifications. It should also avoid setting global config options like the build version if they are not absolutely necessary and ensure that files like a bridging header are handled with care, so they are not overridden but rather extended if already existing.

@saghul
Copy link
Collaborator

saghul commented Nov 21, 2016

@derwaldgeist I understand. I also set the development team, and I do that from a modified hook file. The one we ship is to be considered an example, if you will. All required changes are listed here: https://github.com/eface2face/cordova-plugin-iosrtc/blob/master/docs/Building.md#xcode so one can either use the example hook or write their own as I did.

I would be tempted to just run the hook always as you suggested, but AFAICT there is no way to make all of this "plug and play" (permission descriptions are required, etc) so we might as well document what's needed and have the user choose.

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.

5 participants