Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make ARC default to true #267

Closed
mk opened this Issue · 33 comments

9 participants

@mk
mk commented

I was wondering if we should make the requires_arc flag default to true. I know it would break backwards compatibility but looking ahead I think it could make sense because:

  • when it defaults to false and a library uses arc, you get leaks which I consider to be worse than the other way around: when it defaults to true and a library does not use it, you get build errors
  • more and more libraries are adopting it
  • apple default to it as well
  • we could change all the specs and flip the flag

What do you think?

@alloy
Owner

It makes a lot of sense, but I need to mull it over a bit. Let’s schedule this for after the 0.6.0 release.

@mk
mk commented

Not sure if this is implemented already: if we make this change (or another breaking change to the Specs format) at a later time, can we use the min version from https://github.com/CocoaPods/Specs/blob/master/CocoaPods-version.yml to prompt the user to update his CocoaPods gem in case the Specs repo requires a more recent version than the one installed?

@alloy
Owner

Afaik @irrationalfab implemented all of that feature, so yeah :)

@fabiopelosin

Yes the feature is implemented as described.

Regarding the issue I agree with the proposal, just my two cents.

@alloy
Owner

Ok, then let’s do it.

@mk Will you work on that? And if so, do you think you can do it before the 1st of June (target RC1 release date), or should we schedule it for 0.6.1?

@mk
mk commented

sorry guys, I missed the follow-ups.

@irrationalfab awesome!
@alloy I'll try to look at it tomorrow. Have fun at euruko and good luck with your talk! I won't be able to make it, but @kommen and I will promote CocoaPods at WWDC instead.

@fabiopelosin

Another reason for defaulting ARC to true is that xcodebuild fails (and hence pod spec lint) if it encounters memory management methods while compiling with ARC. With the current behavior instead if you forget to add the flag you end up creating a very leaky pod.

@alloy
Owner

Planned for after 0.14.0. This will also require an update of basically all specs.

@fabiopelosin

Currently (0.17) ARC is required and when all the specs of the master repo have a value, the flag can be changed.

@fabiopelosin

This depends on #960 as it would be the only safe way to set the flag to false on the specs which do not declare it.

(My above comment is incorrect and this is not being checked by the linter)

@kylef
Owner

Maybe it would make sense to change this default for yaml specs. That way the existing DSL specs will continue to work. The DSL>yaml migrator should disable ARC if it wasn't explicitly enabled in the DSL.

This should be a solid way to do this without any existing pods breaking.

@fabiopelosin

The plan is more or less the same: convert programmatically the whole repo to YAML and then explicitly set the value to no for the specs/subspecs which do not specify a value.

@CocoaPodsBot CocoaPodsBot was assigned by mk
@CocoaPodsBot
Collaborator

Issue has been confirmed by @AhmadAlokush

@CocoaPodsBot CocoaPodsBot was unassigned by mk
@CocoaPodsBot CocoaPodsBot was assigned by mk
@CocoaPodsBot CocoaPodsBot was unassigned by mk
@CocoaPodsBot CocoaPodsBot was assigned by mk
@CocoaPodsBot CocoaPodsBot was unassigned by mk
@CocoaPodsBot CocoaPodsBot was assigned by mk
@CocoaPodsBot CocoaPodsBot was unassigned by mk
@CocoaPodsBot CocoaPodsBot was assigned by mk
@CocoaPodsBot CocoaPodsBot was unassigned by mk
@CocoaPodsBot CocoaPodsBot was assigned by mk
@CocoaPodsBot CocoaPodsBot was unassigned by mk
@segiddins
Owner

@Keithbsmiley haven't you done this/made progress on this?

@fabiopelosin

@segiddins before doing this trunk needs to be released and the whole repo must be conveyed to JSON. Marking the issue as blocked.

@Keithbsmiley
Collaborator

The specs are ready for this yes. We can re run my scripts if any new specs have been added that some include this setting

@segiddins
Owner

@Keithbsmiley / @alloy / @irrationalfab ping?

@orta
Owner

I want this, dev pods defaulting to not-ARC is annoying.

@Keithbsmiley
Collaborator

Same

@Keithbsmiley
Collaborator

If anyone wants to do a validate on the specs repo just to make sure nothing snuck in without a definition that might be nice. But last I checked everything was good there.

@segiddins
Owner

@Keithbsmiley care to share your scripts?

@alloy
Owner

I want this too, please write up a list of transition steps/TODOs once you know the status of the conversion script.

@fabiopelosin

Can I have a confirmation that the specs are ready?

@fabiopelosin

172 are still missing the setting according to this simple script:

require 'json'
missing_requires_arc_count = 0
specs = Dir.glob('/Users/fabio/.cocoapods/repos/master/**/*.json')
specs.each do |spec|
  json = JSON.parse(File.read(spec))
  if json['requires_arc'].nil?
    version, name = spec.split('/')[-2..-1]
    puts "#{name} (#{version})"
    missing_requires_arc_count += 1
  end
end

puts "\n\nChecked #{specs.count} specs"
puts "#{missing_requires_arc_count} are missing the requires_arc setting"
@fabiopelosin

Updated script (thanks @kylef):

require 'json'
missing_requires_arc_count = 0
specs = Dir.glob('/Users/fabio/.cocoapods/repos/master/**/*.json')
specs.each do |spec|
  contents = File.read(spec)
  unless contents.include?('requires_arc')
    version, name = spec.split('/')[-2..-1]
    puts "#{name} (#{version})"
    missing_requires_arc_count += 1
    # p spec
  end
end

puts "\n\nChecked #{specs.count} specs"
puts "#{missing_requires_arc_count} are missing the requires_arc setting"
// ...
Checked 17501 specs
130 are missing the requires_arc setting
@Keithbsmiley
Collaborator

Yep. My script isn't going to work for this in it's current state since it was for ruby specs.

@Keithbsmiley
Collaborator

Fixed and pushed CocoaPods/Specs@c50d2b1

@Whirlwind

How should I convert my private repo ? Is there a script to do it?

@orta
Owner

chances are its quicker to just copy and paste s.requires_arc = true into your existing specs

@Whirlwind

too many specs in my repo😁

@alloy
Owner

@Whirlwind See the comments above by @Keithbsmiley and @fabiopelosin.

@Whirlwind

:( only .json, not .spec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.