Improve release script and bower support by gagern · Pull Request #615 · KaTeX/KaTeX

gagern · 2017-01-09T22:09:46Z

I've had a long look at the release script, particularly with the aim of fixing #613. Here is what I changed:

unignore the dist directory for the tag commit
avoid some bower warnings by changing name to lower case and referencing unminified files
drop deprecated version field from bower.json file and the commands to manage it
allow creating releases off branches other than master (e.g. for 0.7.1 from 0.7 branch with bower support?)
clean build and dist directories to avoid stale files lying around
create annotated tags for releases (see Why should I care about lightweight vs. annotated tags? on SO for some discussion)

As these are distinct commits, you might also cherry-pick those you like and reject the others.

I started this off an older master, and had some solution for portable sed. Only then did I notice that 4327e85 already took care of that. I was using this code instead:

# Portable sed -i, see http://unix.stackexchange.com/q/92895/20807
case $(sed --help 2>&1) in
    *GNU*) sedi() { sed -i "$@"; };;
    *) sedi() { sed -i "" "$@"; };;
esac

and then using sedi as a command. Just in case you prefer that over creating then removing a backup file, let me know and I'll create another commit.

xymostech

allow creating releases off branches other than master (e.g. for 0.7.1 from 0.7 branch with bower support?)

I was thinking we should just release 0.7.1 off of master. There haven't been any changes in master since the 0.7.0 release, right? In general I feel like it's bad form to allow releases from non-master. That's the only thing I have an issue with. The rest of the changes look great!

The only other thing I think I would like our release script to do is have a --dry-run option so that we can make sure the run would finish successfully, without actually pushing/making tags/etc. Not sure if it's possible to do that without actually pushing/making tags/etc. though.

xymostech · 2017-01-10T20:31:34Z

bower.json

-    "dist/katex.min.js",
-    "dist/katex.min.css"
+    "dist/katex.js",
+    "dist/katex.css"


Hrm. It's annoying that bower doesn't let us specify several different things with the same filetype. I think people would probably prefer the minified version to be the default? Not sure though.

I couldn't find any documentation about why that requirement exists. bower/bower@1c62adc just says it's following the spec, and https://github.com/bower/spec/blob/master/json.md had that line since the content got copied from Google docs.

I can only guess that they want subsequent steps to minify the sources. The way I understand it, they might even handle modules, so specifying our main unbrowserified entry point in bower.json might work, too.

Yeah, I'm not really sure what main means I guess. It does say that you should use unminified things though, so at least this change is correct.

gagern · 2017-01-10T22:14:21Z

There haven't been any changes in master since the 0.7.0 release, right?

Yes there were: the version bump, and the commit and merge from #612 which theoretically might break things. I deliberately wanted that one for after 0.7.0. I guess it depends on what we want 0.7.x to be. If it's bug fixes only, we should branch a maintainance branch called v0.7 off from 0.7.0^, and release from that. If we use 0.7.x for new features too, the way we'd use a 7.x.0 once we get past the leading zero, then we shouldn't call the development version 0.8.0-pre. Releasing off a non-master branch has the added benefit of actually trying out that script, now that these changes are fresh.

In general I feel like it's bad form to allow releases from non-master.

Why? Patch level releases x.y.z should be from a x.y maintainance branch. That branch itself either forked from master around the x.y.0 release, or from the version x branch if latest development had been at a newer major version by the time x.y.0 got released. At least that's the way I see it.

The only other thing I think I would like our release script to do is have a --dry-run option so that we can make sure the run would finish successfully, without actually pushing/making tags/etc.

Good idea. During development I had created shell functions which just printed the results from the calls to git and npm. Conditionally doing that might be easy. I'll look into it.

xymostech · 2017-01-11T00:19:30Z

Why? Patch level releases x.y.z should be from a x.y maintainance branch. That branch itself either forked from master around the x.y.0 release, or from the version x branch if latest development had been at a newer major version by the time x.y.0 got released. At least that's the way I see it.

Okay yes, that makes sense to me. Maybe my main concern is that we're allowing for running the release script on any branch. I wish we could codify what you just said in code, since I know I've accidentally run the release script from the wrong place before. But it does get printed out now. So I'm fine with this.

Good idea. During development I had created shell functions which just printed the results from the calls to git and npm. Conditionally doing that might be easy. I'll look into it.

Another thing that happened to me was I wasn't logged in to npm, so it got to the place where it made a new tag and then failed to actually do the upload, which required me to manually delete the tag. Maybe we could check that you're logged in as a user with the correct permissions with something like npm owner ls katex | grep $(npm whoami). Just throwing out ideas here. :)

gagern · 2017-01-11T12:08:18Z

Now I've got some sanity checks: a check for uncommited changes, one for the name of the current branch, and one for the npm login. And I've added an option to not exit after a failed sanity check. Do you want it this way? Or should the sanity checks just report the issue, and the user can decide at the confirmation whether or not to ignore them? Should the dry run mode imply not exiting on failed sanity checks? Should the dry run mode indeed execute make, or skip that, too? Should the dry run mode perform all local git operations, then skip pushing them and instead remove the local tag again?

xymostech · 2017-01-11T19:50:10Z

Ahh! Beautiful! I love how --insane and --dry-run work.

Or should the sanity checks just report the issue, and the user can decide at the confirmation whether or not to ignore them?

That might be better if someone just wants to skip one of the checks, but still wants to be warned of other things. That shouldn't be too hard, just editing what $INSANE is set to, right?

Should the dry run mode imply not exiting on failed sanity checks? Should the dry run mode indeed execute make, or skip that, too? Should the dry run mode perform all local git operations, then skip pushing them and instead remove the local tag again?

I like that it executes make, cause that might actually catch bugs. I'm not sure what bugs would be caught by running git for the local operations that aren't already caught by the sanity checks, so I'm happy how this is.

gagern · 2017-01-11T20:03:48Z

That shouldn't be too hard, just editing what $INSANE is set to, right?

I guess in that case I'd make INSANE and --insane go away altogether. Simply echo … >&2 for every failed sanity check, and rely on the fact that a sane user will say no to the subsequent Look good? prompt. Makes the code simpler, too.

I'm not sure what bugs would be caught by running git for the local operations

Operations might fail because a merge is in progress, or a rebase, or because the remote isn't called origin, or because there are some git hooks that do fancy stuff, or because the git doesn't have a user id configured, or a million other reasons I can't think of just now.

Bower prefers to have package names in lower case, and to have uncompressed sources listed as entry points. Otherwise it will issue warnings.

According to bower docs, that field is deprecated and ignored. Git tag names (or commit ids) are the way to identify a given version.

It is customary to use annotated tags for releases, to preserve the information about when the tag itself was created, by whom and for what purpose. In our case we always have a commit directly before the tag, but some workflows may expect annotated tags nonetheless, “git describe” among them. We might want to sign them one day, too.

Now we have some sanity checks, a way to skip them and a way to see which commands would be executed on the git and npm side without actually executing them.

gagern · 2017-01-13T12:56:16Z

As I've just accepted #590 and #591, I've come up with a way to make updating Subresource Integrity hashes part of the release process. I've also dropped the --insane flag, as discussed, relying on a modified prompt instead.

The new script I created for this purpose is making heavy use of ES6 already. I've not subjected it to eslint yet, but intend to do so once #617 has been accepted.

gagern · 2017-01-13T14:32:22Z

How do we proceed here and with 0.7.1? I think this one here should go into master, once you're happy with it. When that happens, we should also merge something like this into the v0.7 branch, which I created by forking off v0.7.0^.

We should decide whether we want to maintain the 0.7 line of development via cherry-picks of bug fixes (whitelist) or via merges from mainline excluding breaking changes which need to be reverted (blacklist). I'd say that after 1.0.0 we should use cherry-picks for patch-level releases and merges for minor-level releases, but prior to 1.0.0 we are more flexible. With the planned switch to ES6 notation, merging large amounts of changes would probably become a pain, though. So I'd suggest we cherry-pick selected commits to v0.7. If you agree I'd do so once this here gets merged into master.

If you want me to, I can try rolling a 0.7.1 release using this updated script. To do so I'll need access to the katex package on npm, though.

gagern · 2017-01-21T22:51:14Z

Thanks for merging, @kevinbarabash! I'll backport this to the v0.7 branch.

k4b7 · 2017-01-21T22:52:45Z

@gagern sorry for the delay. My vote would be for cherry-picks especially since we've already merged the ES6 notation changes.

Improve release script and bower support (cherry picked from commit 8dd161d)

gagern · 2017-01-21T22:57:25Z

Cherry-picked to v0.7 in ad571b9. @kevinbarabash, do you want to build a 0.7.1 release using this? Checking out the v0.7 branch and running the script with 0.7.1 as the only argument should do the trick. And if not, we want to know about it now.

gagern · 2017-01-21T22:58:51Z

Oh, I see I've been granted npm permissions for this. Do you want me to run the release script, to better inspect whether my changes work as intended and so I get some experience doing releases, too?

k4b7 · 2017-01-21T23:00:13Z

release.sh

 npm publish

 if [ ! -z "$NEXT_VERSION" ]; then
    # Go back to master to bump


This comment should be updated.

Next time do the review before the merge… But yes, we should fix this on master, and it doesn't really matter for the 0.7 branch. I guess I'll wait till 0.7.1 is released, just in case there is anything else that needs to be changed.

I don't know the release process that well I figured that you actually running the release script would suss out issues better than I'd be able to reviewing. :(

k4b7 · 2017-01-21T23:02:59Z

release.sh

+    done
    echo "Usage:"
-    echo "./release.sh <VERSION_TO_RELEASE> [NEXT_VERSION]"
+    echo "./release.sh [OPTIONS] <VERSION_TO_RELEASE> [NEXT_VERSION]"


For bug fix releases, would a person run ./release.sh 0.7.1 0.7.2?

I've been asking myself this very question just now. Benefit of doing so is that we get more up-to-date information on the release branch: updated URLs, updated sri hashes, updated version number. The downside is that we'd have one more commit on the branch, but actually that might be seen as a benefit, too, since otherwise it's hard to place the release in the line of development. Do we want to enforce using a second argument?

I could go either way. I'm not too concerned about an extra commit on a release branch.

* Also edit dist/README.md. Otherwise npm publish will overwrite that AFTER the dist directory has been added, causing the git checkout to fail. And it's the right thing to do anyway, having ALL the READMEs edited. * Add all the modified READMEs so they get committed correctly. * Improve two references to master which are no longer accurate. * Check for uncommitted changes just before creating the tag. * Encourage always specifying the next version, as discussed in KaTeX#615 (comment)

This was referenced Jan 9, 2017

Include built files in the repository #114

Closed

0.7.0 no dist folder #613

Closed

gagern force-pushed the release branch from 66a373a to 68307b2 Compare January 10, 2017 08:47

xymostech approved these changes Jan 10, 2017

View reviewed changes

xymostech mentioned this pull request Jan 11, 2017

Revert "Remove trailing commas for IE 9 compatibility" #622

Merged

gagern mentioned this pull request Jan 13, 2017

Allow macro definitions in settings #493

Merged

gagern added 8 commits January 13, 2017 13:38

Un-ignore dist directory on releases for the sake of bower

840771c

Allow creating releases from branches other than master

a3988d9

Avoid some warnings from bower

73cafd1

Bower prefers to have package names in lower case, and to have uncompressed sources listed as entry points. Otherwise it will issue warnings.

Drop version field from bower.json

6404cfd

According to bower docs, that field is deprecated and ignored. Git tag names (or commit ids) are the way to identify a given version.

Add --dry-run and --insane options to release script

961e9db

Now we have some sanity checks, a way to skip them and a way to see which commands would be executed on the git and npm side without actually executing them.

Update CDN version numbers and SRI hashes on release

5783e19

Drop --insane flag, just formulate the prompt appropriately

5e4f50d

gagern force-pushed the release branch from ab1902e to 5e4f50d Compare January 13, 2017 12:49

dkatzberg mentioned this pull request Jan 19, 2017

Upgrade katex to 0.7.0 (bower.json) dzhw/metadatamanagement#930

Closed

k4b7 merged commit 8dd161d into KaTeX:master Jan 21, 2017

gagern pushed a commit that referenced this pull request Jan 21, 2017

Merge pull request #615 from gagern/release

ad571b9

Improve release script and bower support (cherry picked from commit 8dd161d)

k4b7 reviewed Jan 21, 2017

View reviewed changes

gagern mentioned this pull request Jan 21, 2017

Fix release script after experience from 0.7.1 release #642

Merged

Uh oh!

Conversation

gagern commented Jan 9, 2017

Uh oh!

xymostech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagern commented Jan 10, 2017

Uh oh!

xymostech commented Jan 11, 2017

Uh oh!

gagern commented Jan 11, 2017

Uh oh!

xymostech commented Jan 11, 2017

Uh oh!

gagern commented Jan 11, 2017

Uh oh!

gagern commented Jan 13, 2017

Uh oh!

gagern commented Jan 13, 2017

Uh oh!

gagern commented Jan 21, 2017

Uh oh!

k4b7 commented Jan 21, 2017

Uh oh!

gagern commented Jan 21, 2017

Uh oh!

gagern commented Jan 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants