Skip to content

Conversation

dreampiggy
Copy link
Contributor

@dreampiggy dreampiggy commented Jun 11, 2019

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

…rs for libwebp, fix the import issue when using `use_frameworks`
@dreampiggy
Copy link
Contributor Author

@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
Copy link

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
Copy link
Member

@dreampiggy my 2 cents:

  • the whole idea of the subspecs was the ability to link only the ones you want / need. I think the original structure of the podspec was created by someone else (or I can't remember it and I was that person :) ) - anyway, it was a long time ago
  • I'm not a big fan of the prepare_command you used, seems a bit hacky
  • but as long as the solution works for both static library, static framework and dynamic framework, I agree to publish it

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
Copy link
Contributor Author

dreampiggy commented Jun 11, 2019

@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

  • webp: only the code about WebP decoding, without aniamted WebP decoding (need demux) or Color Profile, both of them are in muxer chunk
  • webpencode: contains both WebP decoding and encoding, without animated WebP encoding
  • webpmux: contains the muxer, to encode Animated WebP or custom color profile
  • webpdemux: contains the demuxer, to decode Animated WebP or custom color profile

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
Copy link
Contributor Author

dreampiggy commented Jun 11, 2019

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

…`, `dec`, `demux` and `mux`, all of these subspecs can be compiled without extra dependency
@dreampiggy
Copy link
Contributor Author

@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.

  • dec: Static WebP decoding
  • enc: Static WebP encoding
  • demux: Animated WebP decoding
  • mux: Animated WebP encoding

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
Copy link
Contributor Author

dreampiggy commented Jun 12, 2019

@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:

  • webp: static webp decoding && encoding
  • mux: animated webp encoding
  • demux: animated webp decoding

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

…ubspec includes both WebP encoding && decoding
@dreampiggy dreampiggy force-pushed the fix_cocoapods_frameworks_umbrella_headers branch from 1642707 to 90d20c0 Compare June 12, 2019 08:06
@dreampiggy
Copy link
Contributor Author

dreampiggy commented Jun 12, 2019

😅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
Copy link
Member

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

@bpoplauschi
Copy link
Member

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
Copy link
Contributor Author

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

@dreampiggy dreampiggy merged commit a0ab01d into master Aug 7, 2019
@dreampiggy dreampiggy deleted the fix_cocoapods_frameworks_umbrella_headers branch August 7, 2019 06:42
@bpoplauschi
Copy link
Member

Good @dreampiggy

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