Skip to content

Script to compare the current scaladoc with the parent commit's doc#4765

Merged
SethTisue merged 2 commits intoscala:2.11.xfrom
janekdb:2.11.x-scaladoc-diff
Dec 18, 2015
Merged

Script to compare the current scaladoc with the parent commit's doc#4765
SethTisue merged 2 commits intoscala:2.11.xfrom
janekdb:2.11.x-scaladoc-diff

Conversation

@janekdb
Copy link
Member

@janekdb janekdb commented Sep 23, 2015

This commit was extracted from #4760 to make that PR less diverse.

👌 LGTM on Ubuntu.

Activity Log

This script does not work for me yet. Ideally this would also be tested by others on different UNIXes.

Please pull, test locally and provide comments or patches.

♨️ TODO

  • Fix issue 1. (46f717f)
  • Fix issue 2. (8380096)
  • Fix issue 3.
  • Fix issue 4. (fafa299)
  • Ensure running the script twice copies the scaladoc to the same location. (9373058)
  • Consider issue 5. (0315e78)
  • Fix `issue 6.
  • Check missing new files are detected.
  • Check missing old files are detected.
  • Test on Ubuntu 14.04.3 LTS.
  • Test on OSX - Help welcomed! 🍺 🍺 🍺 🍺
  • Squash commits down to two.
  • Extend any tooling overview documentation that already exists. Outcome: No update required.
  • Take off hold.
  • Add reviewers.
  • Test with git diff instead of displaydiff

🗻 Deferred

  • Add optional param to allow using any ref as the oldsha (HEAD~6 for example)

Known Issues

none

Fixed Issues

✅ Issue 1

OS: Ubuntu 14.04.3 LTS

not found & Bad substitution errors,

21:58 $ ./tools/scaladoc-diff 
tools/get-scala-commit-sha: 11: tools/get-scala-commit-sha: [[: not found
tools/get-scala-commit-sha: 17: tools/get-scala-commit-sha: Bad substitution
parent commit sha: a6c1687aa7. current commit sha: 
making scaladoc for parent commit (a6c1687aa7)
Note: checking out 'HEAD^'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at a6c1687... Merge pull request #4757 from lrytz/t9375-2.11

✅ Issue 2

OS: Ubuntu 14.04.3 LTS

The script uses opendiff which is not present by default on this OS. At the very least the script should preflight the existence of all required commands if there is any reasonable chance they will not be present.

Solution: Check for these commands in order and use the first found,

  • opendiff
  • meld
  • gvimdiff
  • diff -y old new | less

✅ Issue 3

OS: Ubuntu 14.04.3 LTS

ScalaDoc $ ant docs & $ ant docs.lib does not build for @janekdb. Discussion on https://groups.google.com/forum/#!topic/scala-internals/VMzPo_tHL00

Solution: ant clean build

✅ Issue 4

OS: Ubuntu 14.04.3 LTS

The script uses open which is specific to Mac OS X.

Solution: Check for these commands in order and use the first found,

  • open
  • gedit
  • less

open: https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/open.1.html

✅ Issue 5

When the working directory is restored with git checkout $sha the repo ends in a detached head state. Look for a way to restore the starting branch if possible.

  • Determine initial branch
  • If start state was a branch checkout the branch instead of the initial sha when restoring the working directory.

This can be used

git symbolic-ref -q --short HEAD

When on a branch it exits normally with the branch name, if on a detached head there is an error status code and then the short SHA1 can be captured instead.

✅ Issue 6

OS: Ubuntu 14.04.3 LTS

Invoke scaladoc-compare with bash instead of sh to avoid bad substitution error,

sh tools/scaladoc-compare build/scaladoc-${sha}/ build/scaladoc-$oldsha/ &> $difffile

@SethTisue
Copy link
Member

opendiff is an OS X-ism.

@janekdb
Copy link
Member Author

janekdb commented Sep 23, 2015

Here's a few ways we could go on Issue 2,

  1. Document required environment/shell/commands and leave script as-is in respect of opendiff.
  2. Check for required environment/shell/commands and bail if not found.
  3. Generalise the script to Ubuntu at least (and then apply the check above).

I'm happy to go with the option to make the script a bit more portable.

@janekdb janekdb mentioned this pull request Sep 23, 2015
33 tasks
@SethTisue SethTisue added this to the 2.11.8 milestone Sep 24, 2015
@janekdb janekdb force-pushed the 2.11.x-scaladoc-diff branch 3 times, most recently from fafa299 to 9373058 Compare September 28, 2015 20:38
@janekdb janekdb force-pushed the 2.11.x-scaladoc-diff branch 2 times, most recently from 56299ed to 38c7067 Compare October 3, 2015 20:20
Tested on Ubuntu 14.04.3 LTS.

SUMMARY

   1. Use `set -e` to ensure ant failure bails script.
   2. Make best effort to display scaladoc build diff.
   3. Quiet output from git checkout.
   4. Prefer plumbing over porcelain when getting hashes.
   5. Use short hashes to enhance output readability.
   6. Use branch name when available.
   7. Ensure scaladoc copies are clean.
   8. Remove redundant use of scaladoc-compare.
   9. Improve message formatting.
  10. Use $(...) instead of backticks for more legible code.
  11. Pause after reporting missing old file.
  12. Report missing new files.

DETAILS

1. Use `set -e` to ensure an ant failure bails the script. Turn off before
diff because diff returns an error code when the compared files differ,
which is expected to be seen.

2. Make best effort to display scaladoc build diff preferring graphical
clients.

opendiff is a Mac OS X command that opens a graphical diff display, meld
is a graphical client for Ubuntu and other distros. On Ubuntu fallback to
gvimdiff which will display graphically if possible otherwise in the
console. Ultimately default to diff.

Command detection taken from,

  http://stackoverflow.com/questions/592620/check-if-a-program-exists-from-a-bash-script

3. Quiet output from git checkout to avoid detached head warning.

The user does not need to see the detached head warning when running this.

4. Prefer plumbing over porcelain to avoid 'not found' error when getting
SHA1s.

Calling get-scala-commit-sha errors out on Ubuntu 14.04.3 with
Bash 4.3.11 with these messages,

    tools/get-scala-commit-sha: 11: tools/get-scala-commit-sha: [[: not found
    tools/get-scala-commit-sha: 17: tools/get-scala-commit-sha: Bad substitution

5. Use short hashes to enhance output readability.

6. Use branch name when available.

If the branch name is not used when the working directory is restored
after checkout out the parent commit we will be in a detached HEAD
state. Make a best effort to avoid that.

7. Ensure scaladoc copies are clean.

Remove previous copy of scaladoc to ensure consistent behaviour on runs
past the first.

8. Remove use of scaladoc-compare because the equivalent functionality is
provided when iterating the new files.

9. Improve message formatting.

10. Use $(...) instead of backticks for more legible code.

11. Pause after reporting missing old file.

Without this pause it was easy to miss the message when we had this
sequence of differences,

 * differing files
 * missing file
 * differing files

12. Report missing new files.

Along with reporting new files with no corresponding old file report the
complementary scenario.
@janekdb janekdb force-pushed the 2.11.x-scaladoc-diff branch from 38c7067 to 006c0a4 Compare October 3, 2015 20:24
@janekdb
Copy link
Member Author

janekdb commented Oct 3, 2015

Review by @som-snytt, @jsuereth, @SethTisue.

@SethTisue - please take off on-hold.

This has been tested on Ubuntu but not on Mac OS X, so there is a chance an defect has been introduced on OS X. Can anyone test this? This would be enough for a test,

1️⃣ Pull down PR
2️⃣ Edit a word in some Scala doc
3️⃣ Rename some leaf class that won't be missed (this will give a file remove + add)
4️⃣ Commit
5️⃣ Run $ ./tools/scaladoc-diff

You should be greeted by,

1️⃣ A diff of the scaladoc build output
2️⃣ A diff in opendiff of the edited word
3️⃣ Two console messages about missing files

Thanks!

@SethTisue SethTisue removed the on-hold label Oct 3, 2015
@SethTisue
Copy link
Member

this is cool. it worked for me on Mac OS X.

@som-snytt
Copy link
Contributor

I haven't had a chance to try the goodness yet, but I did wonder if we're assuming git why not git diff? Partest prefers git diff because paulp did too.

@janekdb
Copy link
Member Author

janekdb commented Oct 14, 2015

Good point. I'll test with git diff and report back.

@SethTisue
Copy link
Member

@janekdb still interested? I'm finding myself wanting this in order to test #4851

@janekdb
Copy link
Member Author

janekdb commented Nov 25, 2015

@SethTisue The reason this was on-hold was to test the idea of using git diff instead of executing the various diff tools directly. I'd be happy for that to be taken as a later improvement and for this to be merged as-is. It's been tested on Linux (by me) and on Max OS X by you.

Please consider merging.

@lrytz
Copy link
Member

lrytz commented Dec 18, 2015

ping @SethTisue

SethTisue added a commit that referenced this pull request Dec 18, 2015
Script to compare the current scaladoc with the parent commit's doc
@SethTisue SethTisue merged commit 6792b57 into scala:2.11.x Dec 18, 2015
@SethTisue
Copy link
Member

thanks Janek! sorry for the delays.

@SethTisue SethTisue removed the on-hold label Dec 18, 2015
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.

4 participants