SDWebImage / libwebp-Xcode

A wrapper for libwebp + Xcode project. Support Carthage && CocoaPods && SwiftPM.
56 stars 36 forks source link

Rewrite the CocoaPods spec files, to declare the correct Public Headers for libwebp, fix the import issue when using `use_frameworks` #2

Closed dreampiggy closed 5 years ago

dreampiggy commented 5 years ago

Podspec structure issue

Current libwebp.podspec, using this subspec to declare headers:

  s.subspec 'dsp' do |ss|   
    ss.dependency 'libwebp/core'    
  end

I guess the reason is that you want to help to solve this source code import in libwebp

#include "src/dsp/dsp.h"

However, it's useless. The correct and easy way to do so, just keep the original src folder structure. We have Podspec syntax s.preserve_path for this.

Umbrella header

And, curernt libwebp does not works for use_frameworks! and using this code to import

@import SDWebImage;
// Swift
import SDWebImage

Because the umbrella headers generated by CocoaPods, include all the private headers, which have their own macro to detect whether or not to import. For example, neon.h only can be include when build on arm64 device. This should not be exported to public.

So this PR, fix all this problem, and keep compatible with current SDWebImageWebPCoder. And works for use_frameworks! and Swift.

image

dreampiggy commented 5 years ago

@bpoplauschi Another suggestion. Can we change the Git source to libwebp, from

https://chromium.googlesource.com/webm/libwebp

to

https://github.com/webmproject/libwebp

This mirror is faster than the original source. Another reason, for China's company, this googlesource.com maybe blocked in some area and they have to use proxy to use WebP. Using this is more suitable.

The source code between these two repo is the same, because it's just a mirror and maintained by WebM project themselves.

hulizhen commented 5 years ago

This PR perfectly fixes my problem that some header files imported by libwebp itself can't be found in the case of use_frameworks!.

bpoplauschi commented 5 years ago

@dreampiggy my 2 cents:

Can we change the Git source to libwebp, from https://chromium.googlesource.com/webm/libwebp to https://github.com/webmproject/libwebp

I remember we used to change between the chromium and the github sources for the libwebp project (on the main SDWebImage repo). Not sure which is the best, but we can take a look at that too.

dreampiggy commented 5 years ago

@bpoplauschi

the whole idea of the subspecs was the ability to link only the ones you want / need

I check again about the libwebp's their CMakeFile.txt, it has actually, 4 different build target for library

Maybe I can achieve the same thing like those, with 4 subspecs (just need some time to translate the CMake into Podspec :))

But I'm wonder a problem, does this will effect the existing WebP downstream dependency. For example, our SDWebImageWebPCoder, supports both of Animated/Static WebP encoding/decoding. Which means we need all the subspecs. This means the new version libwebp need to specify all the dependency. (Or we can mark default_subspecs contains them all ?)

Another issue, the Subspec like feature is not available for Carthage. Which means, we still need to workaround between these two different integration method. Only CocoaPods users can choose to use a smaller library seems a little, wired 😕

dreampiggy commented 5 years ago

I can have a try now. I'll do my best to split them into 4 subspecs which are both modular and can be compiled seperated. But if I failed on this, we just use one spec.

Actually, current subspecs does not works as you want. If you just use pod 'libwebp/dsp', you can not compile it at all...The same as other util, dec, enc

dreampiggy commented 5 years ago

@bpoplauschi I achieve this goal.

There are 5 subspecs, reprsent the 4 components. The core subspecs does not export anything useful Public API, just used by other subspecs.

All of the combination of subspecs (up to 7 combination), can be compile. And I can run pod lib lint to pass the lint.

image

The Demo project with @import and use_frameworks!

image

dreampiggy commented 5 years ago

@bpoplauschi I test the new podspec again. The 4 subspecs actually, can not pass pob spec lint, only pod lib lint. Because in local file, you can access all other source files. However, libwebp's dec.c file, include the encode.h, for some common macro and inline function usage, make it's not possible to split the encoding and decoding.

So I try again. The result that I can only split them into 3 standalone sub-components:

And I test using the remote git dependency in SDWebImageWebPCoder, with some modification, the standalone webp subspec can work without issue, which only support static WebP. As designed behavior.

platform :ios, '8.0'
use_frameworks!

workspace '../SDWebImageWebPCoder'

target 'SDWebImageWebPCoderExample' do
  pod 'libwebp/webp', :git => 'https://github.com/dreampiggy/libwebp.git', :branch => 'cocoapods_support'
  pod 'SDWebImageWebPCoder', :path => '../'
end

image

dreampiggy commented 5 years ago

😅Seems you're busy now. This patch can fix our feedback users's issue (including myself).

Maybe we need another patch version 1.0.2.1, which use the exist v1.0.2 tag of libwebp, only fix the podspec issue. Not effect Carthage users.

For version, maybe it's not suitable to use something like 1.0.3 or 1.0.3.rc-1, since the code is actually the v1.0.2 code. And I don't want to break current users Podfile.lock (change Podspec will effect the checksum). So this is the simple solution now.

Anyplan to merge this and push to CocoaPods trunk ?

I found myself is not listed in the pod author for this libwebp pod. So I have no right to push one. Could you please help for this ?

➜ pod trunk info libwebp
libwebp
    - Versions:
      - 0.3.0-rc7 (2014-05-19 21:52:10 UTC)
      - 0.3.1 (2014-05-19 21:59:26 UTC)
      - 0.4.0 (2014-05-19 22:02:33 UTC)
      - 0.4.1 (2014-08-08 15:06:06 UTC)
      - 0.4.2 (2014-11-04 08:59:32 UTC)
      - 0.4.3 (2015-03-17 13:26:28 UTC)
      - 0.4.4 (2016-01-18 15:10:52 UTC)
      - 0.5.0 (2016-01-18 15:15:46 UTC)
      - 0.5.1 (2016-09-29 15:21:39 UTC)
      - 0.5.2 (2017-05-05 19:01:52 UTC)
      - 0.6.0 (2017-05-05 19:15:29 UTC)
      - 0.6.1 (2018-04-04 08:07:07 UTC)
      - 1.0.0 (2018-07-31 07:47:06 UTC)
      - 1.0.1 (2019-01-22 11:19:35 UTC)
      - 1.0.2 (2019-01-22 11:29:10 UTC)
    - Owners:
      - Bogdan Poplauschi <bpoplauschi@gmail.com>
      - Shahar Hadas <shaharhd@gmail.com>
bpoplauschi commented 5 years ago

@dreampiggy I just added you as an owner of the CP libwebp proj

bpoplauschi commented 5 years ago

Regarding the issue itself, I don't have the time to investigate it properly, so I'll let you decide. From my point of view, even if we do break compatibility (by removing some subspecs), we do gain the fix so that is a positive thing.

I would not recommend we release virtual versions (that don't exist on the real libwebp repo). Let's just publish 1.0.3 when that becomes available. Until then, I don't see a clear solution to propagate those changes. We could of course PR to the CP specs repo, but they don't accept that for a while now... open to ideas

dreampiggy commented 5 years ago

@bpoplauschi libwebp v1.0.3 is released. I think it's the time to merge this and fix the issue ?

bpoplauschi commented 5 years ago

Good @dreampiggy