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 :checkout option to allow checkinable svn pod when combined with --no-clean #7

Merged
merged 1 commit into from
May 8, 2014

Conversation

yalp
Copy link
Contributor

@yalp yalp commented Dec 12, 2013

The svn downloader uses export instead of checkout since
CocoaPods/CocoaPods#473.

But having a "checkin-able" pod is still usefull for some Pods developpers even
if it has been aggreed to be a minority-use case.
(from CocoaPods/CocoaPods#245 discussion).

Currently with Git, it is just by using --no-clean to pod install or pod update.
With this options, SVN can also have this feature.

The svn downloader uses export instead of checkout by default since
CocoaPods/CocoaPods#473.

But having a "checkin-able" pod is still usefull for the Pods developpers even
if it has been aggreed to be a minority-use case.
(from CocoaPods/CocoaPods#245 discussion).
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling bd3ea5f on yalp:svn-checkout into 9ab9050 on CocoaPods:master.

@alloy
Copy link
Member

alloy commented Dec 12, 2013

This is a smart solution. Just one question, can you add this change to the CHANGELOG and credit yourself?

Do you think we should deprecate :revision and rename it to :export ? /cc @irrationalfab

@yalp
Copy link
Contributor Author

yalp commented Dec 12, 2013

I forgot about the :revision option. I'm not sure it should renamed to :export as it can still be used with :checkout and might be more confusing.

Currently the :checkout option does not use its value and I was confused as in the spec it gives something like this :

 s.source  = { :svn => "https://server/project", :folder => "branch/test", :checkout => "yes_or_whatever" }

Maybe a :mode defaulting to export that could take a checkout value might be cleaner ?

@alloy
Copy link
Member

alloy commented Dec 12, 2013

Ah I see, I had read the patch incorrect. So now the :checkout option would take a boolean value? E.g.

s.source  = { :svn => "https://server/project", :revision => '42', :checkout => true }

And yes, it should default to false to maintain the current behaviour.

@alloy
Copy link
Member

alloy commented Dec 12, 2013

Or, having said that, maybe the option should be :export => false instead of :checkout => true, as some might expect it to already do a checkout, so the use of the word ‘export’ might disambiguate it more.

@fabiopelosin
Copy link
Member

I'm on the fence about this. To my understanding this would allow to use a local podspec or a controlled repo to edit the source in place and support the ability to check-in SVN sources (if the SCM is SVN).

I would be tempted to provide higher level support with a flag like pod install --checkin (or a Podfile directive). Also the options offered by the downloaders, currently, are about what to fetch rather than how. However, in the end, I'm not sure if my proposal would worth it and if it would make sense (what would pod install --checkin do when I'm using a git repo for a SVN Pod). So in the end offering this hackish workaround could be the most appropriate solution.

If we go this route a system patch in cocoapods-core should patch the linter to print a warning if this option is used (to prevent somebody from enabling it in spec submitted to the master repo).

@yalp
Copy link
Contributor Author

yalp commented Dec 13, 2013

@irrationalfab This is exactly my intent. During development, our team currently handles reusable components as git submodules or svn:externals, then we write a podspec and deliver these to other teams as pods.
We don't like having two separate ways to develop and deliver versions of reusable components. We think Cocoapods could replace completely git submodules and svn:extenals mechanisms.

Maybe this should be something a the versioning level, like Maven does with its X.Y.Z-SNAPSHOT feature, that indicates that this version is in dev mode where you get the .svn / .git metadata.

Or maybe we could consider that when we define a pod in a podfile using a repo directly, we are in development mode :

pod 'MyLib', :svn => 'http://server/repo'

whereas having a version is considered as release mode :

pod 'MyLib', '2.6'

@fabiopelosin
Copy link
Member

So the issue is about development mode. I wonder if you investigated the :path option of the pod directive (which would allow such a setup with the caveat that the checkout of the source should be performed manually). There a patch which will introduce the ability to have support for such option via configuration files and later on we plan to introduce a command like pod edit which will automate the process.

Taking into account the above this feature is desirable in the downloader as we need to have an option to specify to preserve the SCM metadata. I would propose to merge and to add a check in cocoapods-core (if you are interested in filing a patch that would be awesome, otherwise please open an issue).

Just in case you didn't consider it. With this patch you will be able to simply do the following as the options are just passed to the downloaders:

pod 'MyLib':svn => "https://server/project", :revision => '42', :checkout => true

@alloy green light for the merge?

@alloy
Copy link
Member

alloy commented Dec 13, 2013

@irrationalfab I agree with everything you said and am 👍 on merging this.

@fabiopelosin
Copy link
Member

Cool. @yalp before merging we need an entry to the changelog and the specs to test the new behavior. Feel free to force push/rebase your branch otherwise I will squash the merge.

@fabiopelosin
Copy link
Member

Any update on this?

@fabiopelosin
Copy link
Member

bump

@yalp
Copy link
Contributor Author

yalp commented Apr 17, 2014

@irrationalfab Sorry, I will not be able to complete this PR myself. I've been, and will be, off the grid all year long for my bike tour around the world ! Sorry again for letting you down on this one.

@fabiopelosin fabiopelosin merged commit bd3ea5f into CocoaPods:master May 8, 2014
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