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

@orta => Move to pre-install hook #31

Merged
merged 18 commits into from Feb 3, 2015

Conversation

Projects
None yet
4 participants
@ashfurrow
Collaborator

ashfurrow commented Feb 1, 2015

So obviously this is pretty bare right now, but it's kind of a proof-of-concept. Was excited to dig into this after a day of walking about Newcastle yesterday, but someone nerd-sniped me with TV Tropes. I'll be submitting little chunks of work as I get them.

I've looked through the CocoaPods codebase (Rainforest is awesome) and am pretty sure that I want to inject my new, faked podspec in the Installer class' install! method, which I've monkey patched here (including a call to the original). This makes sense as a place for pre-install hooks.

I'm still experiencing intermittent installation failures when I call through to the original method. This cafe's wifi is not great, so I'm unable to troubleshoot further (and momentarily getting on a ferry sans Internet). I think it's wise for me to figure out what that's all about before moving on with further monkeys' patches.

Shooting into the Sun is a dirty dirty habit but I just can't help myself.

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Feb 2, 2015

Collaborator

OK, so after talking with @alloy about this, I've got a plan of attack:

  • successfully inject any pod into the installation process
  • generate that pod programmatically, and make it refer to the .m/.h files (generated in next step)
  • use the pod's prepare_command to invoke a ruby script or (hidden?) CocoaPods-keys command that simply generates the .m and .h files in the current working directory (the root of the pod directory)

CocoaPods should then install the keys as a normal pod.

The other great idea (from @mrackwitz) was to force the use of static libraries instead of frameworks (to make the keys more obfuscated, in the main binary). I'm going to go with just what's above first, since we need to get Eidolon in a shipping state. Later, we can worry about improving on that process.

@orta Make sense to you?

@anyone Any gotchas I should be aware of?

Collaborator

ashfurrow commented Feb 2, 2015

OK, so after talking with @alloy about this, I've got a plan of attack:

  • successfully inject any pod into the installation process
  • generate that pod programmatically, and make it refer to the .m/.h files (generated in next step)
  • use the pod's prepare_command to invoke a ruby script or (hidden?) CocoaPods-keys command that simply generates the .m and .h files in the current working directory (the root of the pod directory)

CocoaPods should then install the keys as a normal pod.

The other great idea (from @mrackwitz) was to force the use of static libraries instead of frameworks (to make the keys more obfuscated, in the main binary). I'm going to go with just what's above first, since we need to get Eidolon in a shipping state. Later, we can worry about improving on that process.

@orta Make sense to you?

@anyone Any gotchas I should be aware of?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Feb 2, 2015

Owner

looks legit

Owner

orta commented Feb 2, 2015

looks legit

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

OK, so this obviously needs cleanup, but as a proof-of-concept, I think it's ready to be reviewed. Here's how it works:

  1. Inject a pod called "Keys" (located here) into the installation process.
  2. Override the pod_state in the analyzer to always re-install this pod (in case keys have been changed).
  3. In the from_string method used to parse the podspec from step one, replace %%SOURCE_FILES%% with the names of the source files (generated in step five).
  4. Replace the %%PROJECT_NAME%% with the actual project name (used in the prepare_command to generate the .h/.m files.
  5. The prepare_command is executed, creating the actual .h/.m files the podspec is already referring to.
  6. Installation proceeds as usual.

I have one concern over this strategy: the podspec and the pod name need to be the same, but I'm downloaded the podspec remotely so I can't rename it at runtime (at least, I haven't tried that hard yet). That means that the module containing the keys will always been called Keys. That seems acceptable to me (there is no existing pod called Keys, anyway). However, I'll yield to @orta if he has feels on this.

Collaborator

ashfurrow commented Feb 3, 2015

OK, so this obviously needs cleanup, but as a proof-of-concept, I think it's ready to be reviewed. Here's how it works:

  1. Inject a pod called "Keys" (located here) into the installation process.
  2. Override the pod_state in the analyzer to always re-install this pod (in case keys have been changed).
  3. In the from_string method used to parse the podspec from step one, replace %%SOURCE_FILES%% with the names of the source files (generated in step five).
  4. Replace the %%PROJECT_NAME%% with the actual project name (used in the prepare_command to generate the .h/.m files.
  5. The prepare_command is executed, creating the actual .h/.m files the podspec is already referring to.
  6. Installation proceeds as usual.

I have one concern over this strategy: the podspec and the pod name need to be the same, but I'm downloaded the podspec remotely so I can't rename it at runtime (at least, I haven't tried that hard yet). That means that the module containing the keys will always been called Keys. That seems acceptable to me (there is no existing pod called Keys, anyway). However, I'll yield to @orta if he has feels on this.

@ashfurrow ashfurrow changed the title from [WIP] @orta => Move to pre-install hook to @orta => Move to pre-install hook Feb 3, 2015

Show outdated Hide outdated lib/plugin.rb
require 'cocoapods-core'
require 'keyring_liberator'
require 'key_master'
module CocoaPodsKeys

This comment has been minimized.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Do I even need this (now empty) module?

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Do I even need this (now empty) module?

This comment has been minimized.

@kylef

kylef Feb 3, 2015

Nope, can remove it

@kylef

kylef Feb 3, 2015

Nope, can remove it

Show outdated Hide outdated lib/plugin.rb
CocoaPodsKeys::PreInstaller.new(user_options).setup
# Add our template podspec (needs to be remote, not local).
config.podfile.pod 'Keys', :git => 'https://github.com/ashfurrow/empty-podspec.git', :commit => 'e6588faecb89632f2616251746a313a2ad6336fa'

This comment has been minimized.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

This commit is going to change to a tag – it's just temporary until I can figure out how to replace CocoaPods' cached tag commit (see chatroom).

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

This commit is going to change to a tag – it's just temporary until I can figure out how to replace CocoaPods' cached tag commit (see chatroom).

This comment has been minimized.

@kylef

kylef Feb 3, 2015

Are you talking about VCS locking? You'd just need to do pod update Keys to update it.

@kylef

kylef Feb 3, 2015

Are you talking about VCS locking? You'd just need to do pod update Keys to update it.

Show outdated Hide outdated lib/plugin.rb
if path.to_s.include? "Keys.podspec"
user_options = Pod::Config.instance.podfile.plugins["cocoapods-keys"]
keyring = CocoaPodsKeys::KeyringLiberator.get_keyring_named(user_options["project"]) || CocoaPodsKeys::KeyringLiberator.get_keyring(Dir.getwd)

This comment has been minimized.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

I want to figure out how to abstract a few things here:

  1. Accessing the config from various places in this file (usually for its plugins).
  2. Fully qualifying the class names seems verbose. Want to figure out syntax to not have to do that.
@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

I want to figure out how to abstract a few things here:

  1. Accessing the config from various places in this file (usually for its plugins).
  2. Fully qualifying the class names seems verbose. Want to figure out syntax to not have to do that.

This comment has been minimized.

@kylef

kylef Feb 3, 2015

Podfile's already have an xcodeproj, I don't think it makes sense to introduce a secondary way per-plugin to specify the project.

xcodeproj 'Project.xcodeproj'
plugin 'cocoapods-plugin', 'project' => 'Project.xcodeproj'
@kylef

kylef Feb 3, 2015

Podfile's already have an xcodeproj, I don't think it makes sense to introduce a secondary way per-plugin to specify the project.

xcodeproj 'Project.xcodeproj'
plugin 'cocoapods-plugin', 'project' => 'Project.xcodeproj'

This comment has been minimized.

@alloy

alloy Feb 3, 2015

Contributor

Accessing the config from various places in this file (usually for its plugins).

At the very least you can drop the Pod:: prefix, because you are inside the module Pod … end scope. Other than that, it’s gonna be hard to do so, without adding some kind of global, because the code is spread across multiple module/class scopes.

@alloy

alloy Feb 3, 2015

Contributor

Accessing the config from various places in this file (usually for its plugins).

At the very least you can drop the Pod:: prefix, because you are inside the module Pod … end scope. Other than that, it’s gonna be hard to do so, without adding some kind of global, because the code is spread across multiple module/class scopes.

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Just verified that this runs OK on a device (once you remove Hockey's reference to _mh_execute_header, as we were doing before).

Collaborator

ashfurrow commented Feb 3, 2015

Just verified that this runs OK on a device (once you remove Hockey's reference to _mh_execute_header, as we were doing before).

Show outdated Hide outdated lib/pod/command/keys/generate.rb
interface_file = key_master.name + '.h'
implementation_file = key_master.name + '.m'
File.open(interface_file, 'w') { |f| f.write(key_master.interface) }

This comment has been minimized.

@kylef

kylef Feb 3, 2015

This can be done a bit simpler:

File.write(interface_file, key_master.interface)
@kylef

kylef Feb 3, 2015

This can be done a bit simpler:

File.write(interface_file, key_master.interface)

This comment has been minimized.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Cool – I had copied over some (more complex) code from another of this plugin's files and modified it. Makes sense there'd be an easier way.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Cool – I had copied over some (more complex) code from another of this plugin's files and modified it. Makes sense there'd be an easier way.

Show outdated Hide outdated lib/pod/command/keys/generate.rb
File.open(interface_file, 'w') { |f| f.write(key_master.interface) }
File.open(implementation_file, 'w') { |f| f.write(key_master.implementation) }
else
abort "No keys associated with this directory or project name."

This comment has been minimized.

@kylef

kylef Feb 3, 2015

raise Informative, 'No keys associated with this directory or project name.'
@kylef

kylef Feb 3, 2015

raise Informative, 'No keys associated with this directory or project name.'

This comment has been minimized.

@kylef

kylef Feb 3, 2015

Actually, you should move user input validation to a method called validate! and call help! when the input is wrong.

Example: https://github.com/kylef/cocoapods-deintegrate/blob/master/lib/pod/command/deintegrate.rb#L22-33

@kylef

kylef Feb 3, 2015

Actually, you should move user input validation to a method called validate! and call help! when the input is wrong.

Example: https://github.com/kylef/cocoapods-deintegrate/blob/master/lib/pod/command/deintegrate.rb#L22-33

This comment has been minimized.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Awesome!

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Awesome!

Show outdated Hide outdated lib/plugin.rb
class Installer
include Pod::Podfile::DSL
alias_method :original_install!, :install!

This comment has been minimized.

@alloy

alloy Feb 3, 2015

Contributor

Best to use a less generic name, such as install_before_cocoapods_keys! so that potential chains (other plugins aliasing) won’t break.

@alloy

alloy Feb 3, 2015

Contributor

Best to use a less generic name, such as install_before_cocoapods_keys! so that potential chains (other plugins aliasing) won’t break.

This comment has been minimized.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Good idea – thanks.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Good idea – thanks.

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Addressed feedback and cleans up some concerns I noted earlier.

Collaborator

ashfurrow commented Feb 3, 2015

Addressed feedback and cleans up some concerns I noted earlier.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Feb 3, 2015

lol, dat commit message

alloy commented on d38530c Feb 3, 2015

lol, dat commit message

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Feb 3, 2015

I’d definitely use ‘before’ instead of ‘from’, as you are creating a new method name for the original method.

alloy commented on lib/plugin.rb in 58fcc6d Feb 3, 2015

I’d definitely use ‘before’ instead of ‘from’, as you are creating a new method name for the original method.

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Feb 3, 2015

Owner

Ah, makes sense 👍

Owner

ashfurrow replied Feb 3, 2015

Ah, makes sense 👍

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Feb 3, 2015

I’m starting to think that it might be better if you define e.g. CocoaPodsKeys.podspec_for_current_project() and call that from here. That way you can localise the need to include modules to that scope, instead of changing it globally in the Specification class.

alloy commented on lib/plugin.rb in d38530c Feb 3, 2015

I’m starting to think that it might be better if you define e.g. CocoaPodsKeys.podspec_for_current_project() and call that from here. That way you can localise the need to include modules to that scope, instead of changing it globally in the Specification class.

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Feb 3, 2015

Regarding the use of the Config.instance singleton, you can then extend the CocoaPodsKeys module with the Config::Mixin module and get access to the singleton with just config.

alloy replied Feb 3, 2015

Regarding the use of the Config.instance singleton, you can then extend the CocoaPodsKeys module with the Config::Mixin module and get access to the singleton with just config.

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Feb 3, 2015

Owner

Cool.

Owner

ashfurrow replied Feb 3, 2015

Cool.

Show outdated Hide outdated lib/plugin.rb
PreInstaller.new(user_options).setup
# Add our template podspec (needs to be remote, not local).
config.podfile.pod 'Keys', :git => 'https://github.com/ashfurrow/empty-podspec.git'

This comment has been minimized.

@orta

orta Feb 3, 2015

Owner

Lets get that on this repo 👍

@orta

orta Feb 3, 2015

Owner

Lets get that on this repo 👍

end
end
end
end

This comment has been minimized.

@orta

orta Feb 3, 2015

Owner

what was the motivation for the command?

@orta

orta Feb 3, 2015

Owner

what was the motivation for the command?

This comment has been minimized.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

It is used by the prepare_command in the podspec to generate the .h/.m files in the pod's directory.

@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

It is used by the prepare_command in the podspec to generate the .h/.m files in the pod's directory.

This comment has been minimized.

@orta

orta Feb 3, 2015

Owner

👍

@orta

orta Feb 3, 2015

Owner

👍

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

@orta this is ready for final review/merging. @alloy has some improvements to move code out of the monkey patching back into the plugin, but says he'll submit his own PR.

Collaborator

ashfurrow commented Feb 3, 2015

@orta this is ready for final review/merging. @alloy has some improvements to move code out of the monkey patching back into the plugin, but says he'll submit his own PR.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Feb 3, 2015

Owner

This looks and feels great. And to triple clarify, this work fine with static lib only pods?

Owner

orta commented Feb 3, 2015

This looks and feels great. And to triple clarify, this work fine with static lib only pods?

orta added a commit that referenced this pull request Feb 3, 2015

@orta orta merged commit 2cfffb7 into orta:master Feb 3, 2015

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Can you clarify what you mean? The generated pod doesn't specify to use frameworks or not, so it should work with non-frameworks pods. I'll verify.

Collaborator

ashfurrow commented Feb 3, 2015

Can you clarify what you mean? The generated pod doesn't specify to use frameworks or not, so it should work with non-frameworks pods. I'll verify.

@ashfurrow ashfurrow deleted the ashfurrow:deplooooooooy branch Feb 3, 2015

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Feb 3, 2015

Owner

Coo

Owner

orta commented Feb 3, 2015

Coo

alloy added a commit to alloy/cocoapods-keys that referenced this pull request Feb 3, 2015

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Feb 3, 2015

Collaborator

Verified that this works on installations without frameworks: https://github.com/ashfurrow/static-keys-test (checked in Pods folder).

@orta to quadruple clarify, these changes have not implemented Marius' suggestion to force the pod to be a framework (and thus integrated into the app's binary, further obfuscating the key values). I thought this solution was a reasonable for now (mostly to ship Eidolon) and I could come back and improve it later.

Collaborator

ashfurrow commented Feb 3, 2015

Verified that this works on installations without frameworks: https://github.com/ashfurrow/static-keys-test (checked in Pods folder).

@orta to quadruple clarify, these changes have not implemented Marius' suggestion to force the pod to be a framework (and thus integrated into the app's binary, further obfuscating the key values). I thought this solution was a reasonable for now (mostly to ship Eidolon) and I could come back and improve it later.

alloy added a commit to alloy/cocoapods-keys that referenced this pull request Feb 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment