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

Hash scope suffixes if they are over 50 characters #5608

Merged
merged 1 commit into from
Jul 22, 2016

Conversation

endocrimes
Copy link
Member

@endocrimes endocrimes commented Jul 9, 2016

supersedes CocoaPods/Core#344
actually closes #5491

@segiddins
Copy link
Member

👍🏻

@mrackwitz
Copy link
Member

I don't think we should hash the full label. Just hashing the scope suffix should be sufficient. I can push a PR for that later today.

@endocrimes
Copy link
Member Author

@mrackwitz I can update this PR pretty easily. TBH tho, I don't think we'd really get much of an advantage by doing that?

@endocrimes endocrimes changed the title Hash PodTarget labels >= 60 characters Hash PodTarget suffixes if they are over 30 characters Jul 14, 2016
@endocrimes
Copy link
Member Author

@mrackwitz Updated

@@ -121,7 +123,6 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
[Simon Warta](https://github.com/webmaster128)
[#5595](https://github.com/CocoaPods/CocoaPods/pull/5595)


Copy link
Member

Choose a reason for hiding this comment

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

this second line should be here

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, happened during the rebase, fixing now.

@endocrimes endocrimes force-pushed the dan_filepaths branch 2 times, most recently from 3bcf153 to 011762f Compare July 14, 2016 03:27
@endocrimes
Copy link
Member Author

Build is failing because json is refusing to build on system ruby, but tests are passing elsewhere.
I'm too tired to look into that now, but will take a look in the morning.

@mrackwitz
Copy link
Member

Thanks for updating the PR, @dantoml. 😀
I took a stab at that previously as well, but still didn't came around writing proper specs for it. My idea was to limit the length of the suffix already from the analyzer at the point of the target generation. That's what I came up with:

diff --git a/lib/cocoapods/installer/analyzer/pod_variant_set.rb b/lib/cocoapods/installer/analyzer/pod_variant_set.rb
index b758c31..fd7dc3a 100644
--- a/lib/cocoapods/installer/analyzer/pod_variant_set.rb
+++ b/lib/cocoapods/installer/analyzer/pod_variant_set.rb
@@ -23,7 +23,10 @@ module Pod
         #
         def scope_suffixes
           return { variants.first => nil } if variants.count == 1
-          scope_by_specs
+          Hash[scope_by_specs.map do |variant, scope|
+            scope = Digest::MD5.hexdigest(scope)[0..7] if scope.length > 100
+            [variant, scope]
+          end]
         end

         # Groups the collection by result of the block.

You might want to take over at least the cropping of the hash to 8 chars?

@endocrimes endocrimes changed the title Hash PodTarget suffixes if they are over 30 characters Hash scope suffixes if they are over 50 characters Jul 21, 2016
@endocrimes
Copy link
Member Author

@mrackwitz Updated

@segiddins
Copy link
Member

segiddins commented Jul 22, 2016

👍

@segiddins segiddins merged commit 4e3a738 into CocoaPods:master Jul 22, 2016
@segiddins segiddins deleted the dan_filepaths branch July 22, 2016 18:22
@YoniTsafir
Copy link

@dantoml question about this (sorry if commenting on a closed PR isn't the place but I'm not sure where else to ask): if I want during post_install what target name from the pod's project represents which original PodTarget name, do I have any way of doing so?

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.

PivotalCoreKit Long Directory Name break
4 participants