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

Add a Podfile option to reuse an external source specification #585

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

igor-makarov
Copy link
Contributor

If a dependency was already defined using an external source, reuse that definition:

    # define an external source
    target 'TargetA' do
      pod 'JSONKit', :podspec => 'https://example.com/JSONKit.podspec'
    end
    # reuse it
    target 'TargetB' do
      pod 'JSONKit', :use_previous_external_source => true
    end

@dnkoutso dnkoutso added this to the 1.9 milestone Sep 27, 2019
@segiddins
Copy link
Member

Why does this need to be an option? It seems like we can default to this . behavior?

@dnkoutso
Copy link
Contributor

dnkoutso commented Oct 1, 2019

Agreed. further more this can also be a method inside the Podfile to simplify this?

@igor-makarov
Copy link
Contributor Author

@segiddins - Are we certain that this won't break any existing flow?
@dnkoutso - Can you specify what kind of interface you're imagining?

@igor-makarov igor-makarov force-pushed the external-source-reuse branch from 6e42515 to f29086f Compare October 3, 2019 05:37
@dnkoutso
Copy link
Contributor

dnkoutso commented Oct 3, 2019

def jsonkit_pod
  pod 'JSONKit', :podspec => 'https://example.com/JSONKit.podspec'
end

And then use that method across targets?

@igor-makarov
Copy link
Contributor Author

That's what I'm using right now, but I thought a first-party way would be nicer for everyone.

I do think that @segiddins is right - it can be inferred and not need a separate option. What do you think?

@dnkoutso
Copy link
Contributor

dnkoutso commented Dec 4, 2019

Inferring sounds good.

@igor-makarov
Copy link
Contributor Author

Tried to hack it today, this seems non trivial.

I'll try to find time for it on the weekend.

@igor-makarov igor-makarov force-pushed the external-source-reuse branch from f29086f to d67843a Compare December 7, 2019 11:06
@igor-makarov
Copy link
Contributor Author

Done, figured it out!

Some of the tests were failing because iterating over dependencies mutated the target definitions.
I've changed some of the getters on TargetDefinition to not add a nil value to the hash in the getter. This didn't break anything AFAIK but please review it thoroughly.

@dnkoutso dnkoutso removed this from the 1.9 milestone Jan 30, 2020
if requirements.empty? || requirements.last.is_a?(Hash)
theoretical_dependency = Dependency.new(name, requirements.last)
if theoretical_dependency.external_source.nil?
previously_defined_dependency = dependencies.find do |dependency|
Copy link
Member

Choose a reason for hiding this comment

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

how will this linear scan perform on very large podfiled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a bit of a Shlemiel effect here, but ultimately it's all done in-memory and my profiling for 70 total pods shows an increase from 3% samples to 7%.

What do you think? Should I optimize now or can this be done later? I mean, I've made several optimizations recently that outweigh this by a margin 😂

Copy link
Member

Choose a reason for hiding this comment

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

Probably fine for now, but we might want to eventually put it in a hash for constant-time lookup

@@ -174,7 +174,7 @@ def abstract=(abstract)
# @return [String] the inheritance mode for this target definition.
#
def inheritance
get_hash_value('inheritance', 'complete')
get_hash_value('inheritance') || 'complete'
Copy link
Member

Choose a reason for hiding this comment

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

why these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed several getters to avoid mutating the hash because iterating over deps mutated them.

The code assumed, previously, (and implicitly) that deps only get evaluated after the Podfile is completely scanned.

However, going over previously added deps before the entire Podfile is scanned mutated things and now it doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

👍. Is there a test that will fail if we accidentally go back to the old behavior later?

Copy link
Contributor

Choose a reason for hiding this comment

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

want to continue this @igor-makarov ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hadn't had the time to think about this yet.. Sorry 😭

@igor-makarov igor-makarov force-pushed the external-source-reuse branch from 77365e0 to b64674c Compare January 30, 2021 15:28
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.

3 participants