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 Cocoapods podspec generation. #568

Merged
merged 3 commits into from Aug 14, 2018

Conversation

Projects
None yet
3 participants
@Lukasa
Contributor

Lukasa commented Aug 10, 2018

Motivation:

A number of users have asked for the ability to install SwiftNIO
via Cocoapods. This is an entirely reasonable request, so let's
do it.

Modifications:

  • Added a script that can generate, and optionally upload,
    podspecs.
  • Made a slight modification to the dependency script to provide
    target-specific dependencies.

Result:

Users can use cocoapods if they prefer.

@Lukasa Lukasa added this to the 1.10.0 milestone Aug 10, 2018

@Lukasa Lukasa requested a review from weissi Aug 10, 2018

@weissi

thanks, looks pretty good, just some shell nits :)

}
upload=false
while getopts ":u" opt; do

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor
  • I think you don't want the colon in there, right? The colon (usually after an option name) means that the option gets an argument. Or am I missing something.
  • Also I'd recommend to do OPTIND=1 before the while. Just in case someone was nasty enough to export OPTIND=7 or so before running the script

This comment has been minimized.

@Lukasa

Lukasa Aug 10, 2018

Contributor

The colon means "please don't output your own error messages if the flags are wrong".

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor

@Lukasa ah thanks, TIL! Also: very very obvious syntax :P

This comment has been minimized.

@Lukasa

Lukasa Aug 10, 2018

Contributor

It's shell man, what do you want? 😉

# SwiftX or CX, but our target names aren't. We add SwiftX to the front
# of all NIO targets, and then remove it in the source files declaration
# if needed.
TARGETS=$(${here}/list_topsorted_dependencies.sh -l -r | grep -v "NIOPriorityQueue" | sed 's/^NIO/SwiftNIO/')

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor

that needs to be quoted, maybe run shellcheck

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor

and I think if you do

targets=( $( ... ) )

then targets becomes an array

# if needed.
TARGETS=$(${here}/list_topsorted_dependencies.sh -l -r | grep -v "NIOPriorityQueue" | sed 's/^NIO/SwiftNIO/')
for target in $TARGETS; do

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor

once targets is an array you can loop like for target in "${targets[@]}"; do

exit 1
fi
VERSION=$1

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor

we should use lower case, upper case are reserved by shell for stuff with special meaning

# We need to suppress the return code of grep here.
RAW_DEPENDENCIES=$(${here}/list_topsorted_dependencies.sh -d "${target#Swift}" | grep -v "NIOPriorityQueue" | sed 's/^NIO/SwiftNIO/')
DEPENDENCIES=""

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor

this should be an array

dependencies=()
DEPENDENCIES=""
for raw_dependency in $RAW_DEPENDENCIES; do
DEPENDENCIES="${DEPENDENCIES}${NEWLINE} s.dependency '$raw_dependency', s.version.to_s"

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor

this should be array append

dependencies+=( "${dependencies}${newline}  s.dependency '$raw_dependency', s.version.to_s" )
echo "Building podspec for $target"
# We need to suppress the return code of grep here.
RAW_DEPENDENCIES=$(${here}/list_topsorted_dependencies.sh -d "${target#Swift}" | grep -v "NIOPriorityQueue" | sed 's/^NIO/SwiftNIO/')

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor

generally I think looping over output is better done like this:

while read -r line; do
    # process each line
done < <(command that output lines)
@@ -53,6 +58,12 @@ if $do_reversed; then
transform=tac_compat
fi
if [[ ! -z "$package_dependency" ]]; then
swift package dump-package | jq -r ".targets |
map(select(.name == \"$package_dependency\" and .type == \"regular\") | .dependencies | map(.name)) | .[] | .[]"

This comment has been minimized.

@weissi

weissi Aug 10, 2018

Contributor

can we break that jq in multiple lines (maybe after each |)?

Add Cocoapods podspec generation.
Motivation:

A number of users have asked for the ability to install SwiftNIO
via Cocoapods. This is an entirely reasonable request, so let's
do it.

Modifications:

- Added a script that can generate, and optionally upload,
  podspecs.
- Made a slight modification to the dependency script to provide
  target-specific dependencies.

Result:

Users can use cocoapods if they prefer.
@Lukasa

This comment has been minimized.

Contributor

Lukasa commented Aug 10, 2018

Feedback addressed.

@weissi

weissi approved these changes Aug 10, 2018

beautiful :)

weissi added some commits Aug 14, 2018

@weissi weissi merged commit 2844096 into apple:master Aug 14, 2018

2 checks passed

pull request validation (4.0.3) Build finished.
Details
pull request validation (4.1) Build finished.
Details

@weissi weissi modified the milestones: 1.10.0, 1.9.3 Aug 28, 2018

weissi added a commit that referenced this pull request Aug 29, 2018

Add Cocoapods podspec generation. (#568)
Motivation:

A number of users have asked for the ability to install SwiftNIO
via Cocoapods. This is an entirely reasonable request, so let's
do it.

Modifications:

- Added a script that can generate, and optionally upload,
  podspecs.
- Made a slight modification to the dependency script to provide
  target-specific dependencies.

Result:

Users can use cocoapods if they prefer.
Motivation:

Explain here the context, and why you're making that change.
What is the problem you're trying to solve.

Modifications:

Describe the modifications you've done.

Result:

After your change, what will change.
@fcorneli

This comment has been minimized.

fcorneli commented Oct 7, 2018

Could this be updated for version 1.9.5? Only 1.9.2 is available for now.

@weissi

This comment has been minimized.

Contributor

weissi commented Oct 7, 2018

@fcorneli thanks for the report, updating them right now! I'll also add that to our release checklist so we shouldn't forget in the future...

@weissi

This comment has been minimized.

Contributor

weissi commented Oct 7, 2018

ok, 1.9.5. uploaded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment