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.

Sign up for GitHub

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

Jump to bottom

Support CocoaPods #70

Merged
merged 2 commits into from
Aug 14, 2017
Merged

Support CocoaPods #70

merged 2 commits into from
Aug 14, 2017
+54 −6

Conversation

simorgh3196
Copy link
Contributor

@simorgh3196 simorgh3196 commented Aug 12, 2017 •
edited

Issue: #52

I hope I can install this library with CocoaPods.

I tried installing with CocoaPods by referring to SwiftLint etc.
I did not actually register with CocoaPods, so I can not confirm whether it will work, but I tried making a PR wishing this would be useful.

I would be happy if you can provide feedback if there are still missing operations.

TODO

  • Register CocoaPods
  • Installation Test

@mono0926
Copy link
Owner

mono0926 commented Aug 12, 2017 •
edited

Thanks, very much!
It seems to be great.
I have never supported CocoaPods distribution, so I postponed #52. And so, this PR is very helpful 😋

TODO

  • Register CocoaPods
  • Installation Test

How should I treat this TODO?

Do you demand me to merge to master to deal with them?
If so, I'll merge this.

In advance, I noticed a review point, so I'll comment now.

mono0926
mono0926 requested changes Aug 12, 2017
View reviewed changes
Makefile Outdated
mkdir "$(TEMPORARY_FOLDER)"
cp -f ".build/release/LicensePlist" "$(TEMPORARY_FOLDER)"
cp -f "LICENSE" "$(TEMPORARY_FOLDER)"
(cd tmp/portable_licenseplist; zip -r - LICENSE LicensePlist) > './portable_licenseplist.zip'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is tmp/portable_licenseplist correct?
It seems to be that $(TEMPORARY_FOLDER)(tmp_portable_licenseplist ) is correct for me.

mono0926
mono0926 requested changes Aug 13, 2017
View reviewed changes
Makefile
clean:
swift package clean

xcode:
swift package generate-xcodeproj


install: build
mkdir -p "$(PREFIX)/bin"
cp -f ".build/release/LicensePlist" "$(PREFIX)/bin/license-plist"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The executable binary is renamed to chain case from camel case here when installed via Makefile or Homebrew.
It is better to also apply it cp -f ".build/release/LicensePlist" "$(TEMPORARY_FOLDER)", isn't it?
(I don't understand CocoaPods distribution well.)

Makefile Outdated

portable_zip: build
mkdir "$(TEMPORARY_FOLDER)"
cp -f ".build/release/LicensePlist" "$(TEMPORARY_FOLDER)"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be cp -f ".build/release/LicensePlist" "$(TEMPORARY_FOLDER)/license-plist"?

README.md Outdated
```sh
if [ $CONFIGURATION = "Debug" ]; then
cd $SRCROOT
${PODS_ROOT}/LicensePlist/portable_licenseplist --output-path $PRODUCT_NAME/Settings.bundle
Copy link
Owner

@mono0926 mono0926 Aug 13, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of SwiftLint CocoaPods distribution, its executable binary is located at Pods/SwiftLint/swiftlint.

So, I think ${PODS_ROOT}/LicensePlist/license-plist --output-path $PRODUCT_NAME/Settings.bundle is more natural.
(Of course, it is necessary to modify other scripts to fit this command if needed.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I rename binary name to license-plist, also modify others.

mono0926
mono0926 reviewed Aug 13, 2017
View reviewed changes
LicensePlist.podspec.tmp Outdated
s.license = { :type => 'MIT', :file => 'LICENSE' }
s.author = 'Masayuki Ono'
s.source = { :http => '#{s.homepage}/releases/download/#{s.version}/portable_licenseplist.zip' }
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do this file name contain .tmp?
I think you can test CocoaPods installation with this command by removing .tmp.

pod 'LicensePlist', :git => 'https://github.com/simorgh3196/LicensePlist.git', :branch => 'cocoapods'

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s.source = { :http => '#{s.homepage}/releases/download/#{s.version}/portable_licenseplist.zip' }

I noticed that, it is required to modify this for testing temporarily.
If it is difficult to test in advance, I'll merge it after the review requests is processed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to deliver CocoaPods updates in release.sh, I wanted to be able to set the version number externally.

Therefore, I thought that create a new podspec dynamicary, and replace version number.

LicensePlist.podspec.tmp

s.version = 'LATEST_RELEASE_VERSION_NUMBER'

release.sh

cat "$podspec_name.tmp" | sed s/LATEST_RELEASE_VERSION_NUMBER/$tag/ > "$podspec_name"

If you have any other good ideas please let me know.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I understand. It seems to be good 👍

(SwiftLint use another technique though.)

mono0926
mono0926 approved these changes Aug 14, 2017
View reviewed changes
LicensePlist.podspec.tmp Outdated
s.license = { :type => 'MIT', :file => 'LICENSE' }
s.author = 'Masayuki Ono'
s.source = { :http => '#{s.homepage}/releases/download/#{s.version}/portable_licenseplist.zip' }
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I understand. It seems to be good 👍

(SwiftLint use another technique though.)

@mono0926 mono0926 merged commit 8f99b32 into mono0926:master Aug 14, 2017
mono0926 added a commit that referenced this pull request Aug 14, 2017
@mono0926
Revert README( #70 ) temporarily
f30208f 
- After confirmed that CocoaPods distribution works fine, I'll re-revert this.

This error occurred:

```sh
==> make install PREFIX=/usr/local/Cellar/license-plist/1.6.0
🍺  /usr/local/Cellar/license-plist/1.6.0: 5 files, 9.3MB, built in 50 seconds
  adding: license-plist (deflated 72%)
swift build -c release -Xswiftc -static-stdlib
mkdir -p "./tmp_portable_licenseplist"
cp -f ".build/release/LicensePlist" "./tmp_portable_licenseplist/license-plist"
cp -f "LICENSE" "./tmp_portable_licenseplist"
(cd ./tmp_portable_licenseplist; zip -r - LICENSE license-plist) > "./portable_licenseplist.zip"
  adding: LICENSE (deflated 41%)
  adding: license-plist (deflated 72%)
rm -r "./tmp_portable_licenseplist"
cat: LicencePlist.podspec.tmp: No such file or directory
[!] You need to register a session first.
```
@mono0926
Copy link
Owner

Thank you for the PR.

I noticed that you tested the installation: https://github.com/simorgh3196/LicensePlist/tree/cocoapods-test 👍

After I merged this to master, I executed ./release.sh 1.6.0 GITHUB_TOKEN but it failed.
So I reverted README temporarily.
See this commit message: f30208f

If you could resolve this error, submit a new PR.
Or I'll try to resolve it later.

@Econa77 Econa77 mentioned this pull request Dec 14, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mono0926 mono0926 mono0926 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@simorgh3196 @mono0926