duemunk / Async

Syntactic sugar in Swift for asynchronous dispatches in Grand Central Dispatch
MIT License
4.59k stars 316 forks source link

Fix missing CFBundleVersion when built with Carthage (Reconfigured the whole project as Universal Target) #108

Closed toshi0383 closed 7 years ago

toshi0383 commented 8 years ago

Fixes #106

See Carthage/Carthage#517 for details.

The test target setting was causing unnecessary build for AsyncTest scheme.

AsyncTest/Info.plist's CFBundleVersion is "$(CURRENT_PROJECT_VERSION)", which ended-up with empty string ("") because AsyncTest/Async target's CURRENT_PROJECT_VERSION is empty string ("").

That means all carthage users had been using the wrong target. Scary...

codecov-io commented 8 years ago

Current coverage is 97.79% (diff: 100%)

No coverage report found for master at b7cde33.

Powered by Codecov. Last update b7cde33...5723077

toshi0383 commented 8 years ago

Fixing podspec and realized... Why does Async not support watchOS 2.* ?

toshi0383 commented 8 years ago

Sorry guys, I rebased a lot. It should be good to go now. I recreated the whole xcodeproj, so it needs to be tested. Other than that, carthage build is now working correctly for CFBundleVersion.

Async $ carthage build --no-skip-current
Async $ for i in `find Carthage/Build/ -name "*plist"` ;do result=`plutil -p ${i}|grep CFBundleVersion`;echo "$result" ${i}; done
  "CFBundleVersion" => "1.0" Carthage/Build//iOS/Async.framework/Info.plist
  "CFBundleVersion" => "1.0" Carthage/Build//iOS/Async.framework.dSYM/Contents/Info.plist
  "CFBundleVersion" => "1.0" Carthage/Build//Mac/Async.framework/Versions/A/Resources/Info.plist
  "CFBundleVersion" => "1.0" Carthage/Build//Mac/Async.framework.dSYM/Contents/Info.plist
  "CFBundleVersion" => "1.0" Carthage/Build//tvOS/Async.framework/Info.plist
  "CFBundleVersion" => "1.0" Carthage/Build//tvOS/Async.framework.dSYM/Contents/Info.plist
  "CFBundleVersion" => "1.0" Carthage/Build//watchOS/Async.framework/Info.plist
  "CFBundleVersion" => "1.0" Carthage/Build//watchOS/Async.framework.dSYM/Contents/Info.plist
duemunk commented 7 years ago

Can you split this in to single purpose pull requests?

toshi0383 commented 7 years ago

@duemunk Thanks for reviewing. IMHO, this is actually a single purpose PR. All of this structure change is needed for Carthage compatibility.

duemunk commented 7 years ago

I've (finally) taken a look at it locally :)

  1. Why is Source renamed to Sources
  2. Why have the tests been moved to the Carthage project?
  3. The project in AsyncTest/Async.xcodeproj doesn't work anymore?
toshi0383 commented 7 years ago

Thanks for feedback!

  1. Why is Source renamed to Sources

SwiftPM's generate-xcodeproj command imports all source files into a group named Sources on xcodeproj. I renamed the physical directory name before running generate-xcodeproj command to avoid confusion.

  1. Why have the tests been moved to the Carthage project?

As mentioned in Carthage/Carthage#517, carthage tries to build every xcodeproj. So having multiple xcscheme shared is troublesome. As I mentioned at top, this is what happened for CFBundleVersion's value this time.

AsyncTest/Info.plist's CFBundleVersion is "$(CURRENT_PROJECT_VERSION)", which ended-up with empty string ("") because AsyncTest/Async target's CURRENT_PROJECT_VERSION is empty string ("").

Maybe setting CURRENT_PROJECT_VERSION value solves this temporarily, but I thought I should resolve the root cause to avoid any similar issues in future.

This is why I moved everything into a single xcodeproject. This is what SwiftPM does by default, too.

  1. The project in AsyncTest/Async.xcodeproj doesn't work anymore?

No, it does not. I deleted it and merged it into ./Async.xcodeproj using swift package generate-xcodeproj command.

This PR indeed looks big, but all I did is pretty simple straight forward.

toshi0383 commented 7 years ago

🚢

duemunk commented 7 years ago

Thanks for your work @toshi0383! Sorry for not being more responsive. Also, you clearly know what you're doing, I just couldn't follow right away :)