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.

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

Auto-update version and bundle number using Git #208

Merged
merged 3 commits into from Feb 4, 2015
Merged

Conversation

kaishin
Copy link

@kaishin kaishin commented Dec 17, 2014

No description provided.

@gfontenot
Copy link
Member

Turns out, this isn't enough. We need to modify these values in the generated dSYM as well. Not sure if there is a variable set for that info.plist.

@kaishin
Copy link
Author

kaishin commented Jan 8, 2015

I have't looked at this closely but there seems to be ways to do that manually http://stackoverflow.com/questions/13323728/update-cfbundleshortversionstring-in-dsym-at-build

I'm not sure how I was able to use Hockey with this script without touching the dSYM info.plist...

@gfontenot
Copy link
Member

@kaishin I modified the script to update the dSYM as well.

Can I get a sanity check from @thoughtbot/shell on this script for correctness?

@@ -0,0 +1,16 @@
GIT=$(sh /etc/profile; which git)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a shebang? #!/bin/sh

Copy link
Contributor

Choose a reason for hiding this comment

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

No shebang because the shell this is run through is set in Xcode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK. This has more UPPERCASE variable names than I usually see, but that's not really a huge deal.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I meant to lowercase everything. Doing that now.

@keith
Copy link
Contributor

keith commented Feb 4, 2015

Quick note on this for the future, this same approach doesn't work on OS X. The app refuses to build if the built product has a different build number than the project.

@@ -0,0 +1,16 @@
GIT=$(sh /etc/profile; which git)
GIT_RELEASE_VERSION=$(${GIT} describe --tags --always --abbrev=0)
COMMITS=$(${GIT} rev-list HEAD | wc -l)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about calling this NUMBER_OF_COMMITS?

Copy link

Choose a reason for hiding this comment

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

Should quote $GIT to be safe.

Copy link
Member

Choose a reason for hiding this comment

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

I have a change staged where I quoted the entire assignment. Should I do both?

Copy link

Choose a reason for hiding this comment

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

Assignments don't need quoting, but I often quote them anyway because it doesn't hurt. Up to you what you want to do here, the minimum, safe, POSIX, thing would be:

commits=$("$git" rev-list HEAD | wc -l)

Copy link
Contributor

Choose a reason for hiding this comment

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

Pat and I discovered yesterday that you don't need to quote assignment, ever, in any of the popular shells.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should still quote GIT.

Copy link

Choose a reason for hiding this comment

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

Note that quoting around "$( ... )" does not mean you don't still have to quote any variables inside.

@gfontenot
Copy link
Member

@Keithbsmiley yikes. Should we bail on this entirely? Or should we keep it since Liftoff is still iOS only?

fi
done

echo "Updated ${TARGET_BUILD_DIR}/${INFOPLIST_PATH}"
Copy link

Choose a reason for hiding this comment

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

The curly braces are not required here. Also, echo with variables is very un-portable. I'd suggest

printf "Updated %s/%s\n" "$TARGET_BUILD_DIR" "$INFOPLIST_PATH"

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually ditching the echo, since I don't think it's going to be super useful.

@keith
Copy link
Contributor

keith commented Feb 4, 2015

I think this is fair to leave in here for now. I think that script may have to differ by platform regardless. But yea that was just meant to be food for thought, since OS X support is a while off if at all.

for PLIST in $TARGET_PLIST $DSYM_PLIST
do
if [ -f "$PLIST" ] ; then
/usr/libexec/PlistBuddy -c "Set :CFBundleVersion ${COMMITS}" "$PLIST"
Copy link

Choose a reason for hiding this comment

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

Curly brances aren't required on ${COMMITS}.

In general I don't mind when folks want to always use curly braces on all variables, but you should be consistent throughout the script, or only use them when required (like ${GIT_RELEASE_VERSION#*v})

@gfontenot
Copy link
Member

Updated with fixes from comments. @pbrisbin @Keithbsmiley @gabebw Mind giving this another once over?

@@ -0,0 +1,12 @@
git=$(sh /etc/profile; which git)
git_release_version=$("$git" describe --tags --always --abbrev=0)
commits=$("$git" rev-list head | wc -l | tr -d ' ')
Copy link
Contributor

Choose a reason for hiding this comment

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

how about calling this number_of_commits?

Copy link
Member

Choose a reason for hiding this comment

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

oh right, missed that one.

@keith
Copy link
Contributor

keith commented Feb 4, 2015

👍

@gabebw
Copy link
Contributor

gabebw commented Feb 4, 2015

I approve the shell.

@pbrisbin
Copy link

pbrisbin commented Feb 4, 2015

Very nice.

@@ -32,6 +32,8 @@ perform the following configurations:
* Turn on Static Analysis for the project.
* Add a build phase shell script that [turns "TODO:" and "FIXME:" into
warnings][deallocated-todo].
* Add a build phase shell script that sets the version and build number
using respectively the latest Git tag and the number of commits.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe: "sets the version to the latest Git tag and the build number to the number of commits"?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's better.

This script updates the version info for the build application by
modifying the plist in the biuld binary. It also updates the plist for
the generated dSYM to ensure that they match.

The CFBundleVersion is set based on the number of commits on master
The CFBundleShortVersionString is set based on the most recent git tag
@gfontenot gfontenot merged commit fd04483 into master Feb 4, 2015
@gfontenot gfontenot deleted the rl-version-number branch February 4, 2015 18:30
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.

None yet

6 participants