Tuesday, November 6, 2012

Submitting a Code Fix for the Dart SDK

‹prev | My Chain | next›

I have spent quite a few days working on a two character bug fix for the dartdoc tool, which is part of the Dart SDK. Actually, I found the fix on the first day, but had me a good old time rooting through the code and figuring out how to test it. But finally, I am ready to submit my code to the the Dart project.

I signed the Google Individual Contributor License Agreement yesterday.

Next, I need to install the “depot tools”, which are used for code review submissions:
➜  repos  git clone https://git.chromium.org/chromium/tools/depot_tools.git
I add that repository to my PATH:
PATH=$HOME/repos/depot_tools:$PATH
Next, I have to grab the Dart source. I cloned the GitHub mirror the other day, but it seems like I need the “real” source for this. Since the contribution instructions refer to using git, I will follow along with git section of the “getting the source“ documentation:
➜  repos  svn ls http://dart.googlecode.com/svn/branches/bleeding_edge/
AUTHORS
LICENSE
PATENTS
dart/
deps/
➜  repos  mkdir dart-repo
➜  repos  cd dart-repo
➜  dart-repo  gclient config http://dart.googlecode.com/svn/branches/bleeding_edge/deps/all.deps
➜  dart-repo  git svn clone -rHEAD http://dart.googlecode.com/svn/branches/bleeding_edge/dart dart
➜  dart-repo  gclient sync
➜  dart-repo  gclient runhooks
Then, I follow the get-latest-changes procedure (presumably I don't really need this since this is newly initialized):
➜  dart-repo  cd dart
➜  dart git:(master) git svn fetch
➜  dart git:(master) git merge git-svn
➜  dart git:(master) gclient sync
➜  dart git:(master) ✗ gclient runhooks
After the gclient sync command and after runhooks, I have several files lying around:
dart git:(master) ✗ gst
# On branch master
# Untracked files:
#   (use "git add ..." to include in what will be committed)
#
#       compiler/dart_analyzer.target.mk
#       dart2js_bot.target.mk
#       samples.target.mk
#       samples/sample_extension/sample_extension.Makefile
#       samples/sample_extension/sample_extension.target.mk
nothing added to commit but untracked files present (use "git add" to track)
Those do not seem to have any effect on things so I get started on my changes. It seems that no one has fixed my newline fix yet, so I add the change to my repository:
➜  dart git:(master) ✗ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD ..." to unstage)
#
#       modified:   sdk/lib/_internal/dartdoc/lib/src/dartdoc/comment_map.dart
#       new file:   sdk/lib/_internal/dartdoc/test/comment_map_test.dart
I make sure that my new test passes for me:
➜  dart git:(master) ✗ ./tools/testing/bin/linux/dart sdk/lib/_internal/dartdoc/test/comment_map_test.dart
unittest-suite-wait-for-done
PASS: triple slashed comments retain newlines

All 1 tests passed.
unittest-suite-success
Then I “break” the import statement for the unittest.dart library to conform to the same relative path that the other tests in the same directory use. I still have not figured out an explanation for the different paths, but hopefully this will work on the Dart build system. I commit the fix:
➜  dart git:(master) ✗ git co -b code-samples-for-triple-slash-dartdoc
M       sdk/lib/_internal/dartdoc/lib/src/dartdoc/comment_map.dart
A       sdk/lib/_internal/dartdoc/test/comment_map_test.dart
Switched to a new branch 'code-samples-for-triple-slash-dartdoc'
➜  dart git:(code-samples-for-triple-slash-dartdoc) ✗ git ci -m "Fix for triple-slash dartdocs    
dquote> 
dquote> They had been ignoring multiline markdown such as code samples."


They had been ignoring multiline markdown such as code samples."[code-samples-for-triple-slash-dartdoc 1093824] Fix for triple-slash dartdocs
 2 files changed, 36 insertions(+), 1 deletion(-)
 create mode 100644 sdk/lib/_internal/dartdoc/test/comment_map_test.dart
Next, I create myself an account on http://chromiumcodereview.appspot.com/.

At this point, I am ready to begin my “change list”. I configure the process with git cl config:
➜  dart git:(code-samples-for-triple-slash-dartdoc) ✗ git cl config
Rietveld server (host[:port]) [https://codereview.chromium.org/]: http://chromiumcodereview.appspot.com
CC list ("x" to clear) [reviews@dartlang.org]:
Tree status URL:
ViewVC URL ("x" to clear) [https://code.google.com/p/dart/source/detail?r=]:
Now, I start the upload process:
➜  dart git:(code-samples-for-triple-slash-dartdoc) ✗ git cl upload
Using 50% similarity for rename/copy detection. Override with --similarity.
Running presubmit upload checks ...

Presubmit checks passed.
 .../dartdoc/lib/src/dartdoc/comment_map.dart       |    2 +-
 .../_internal/dartdoc/test/comment_map_test.dart   |   35 ++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)
That pops up an editor asking for a description of the patch. It is pre-populated with the commit message, but I add a bit more:
# Enter a description of the change.
# This will displayed on the codereview site.
# The first line will also be used as the subject of the review.
Fix for triple-slash dartdocs

They had been ignoring multiline markdown such as code samples.
More information available at:
http://japhr.blogspot.com/2012/11/troubleshooting-triple-slash-dartdocs.html

This also includes a new test file, though I am unsure if the 
paths are correct.

BUG=
After saving that, I am prompted for the password for my account. It takes me a while to figure out that I need to generate an application specific password as described in http://support.google.com/accounts/bin/answer.py?hl=en&answer=185833. The prompt kept telling me to do this, but for some reason I did not register that I need to go through the generation process.

Once I sorted out my password issue, I was able to submit the “change list”, which returned:
Issue created. URL: https://codereview.chromium.org/11358134
Uploading base file for sdk/lib/_internal/dartdoc/lib/src/dartdoc/comment_map.dart
Uploading base file for sdk/lib/_internal/dartdoc/test/comment_map_test.dart
That change list is, indeed, available at: https://codereview.chromium.org/11358134.

Armed with that URL, I can now file a bug report at http://dartbug.com: http://code.google.com/p/dart/issues/detail?id=6584. I am unsure how to attach the “change list” to the Dart bug, so I add it in a comment.

And that should do it. I am reasonably confident that the actual fix is solid. I am less sure about the test (paths to libraries and style). Hopefully I have completed all of the necessary steps correctly.


Day #562

2 comments:

  1. You will have to do one more thing: publish the change list for review. You will have to select reviewers, which should be people familiar with the code (git blame will help). For dartdoc, that would be Bob and I think at least one other person (Johnni Winter? Not sure now). You can also fill a message for reviewers, but since the email for them will already contain the change list description, you can leave it empty for such small change. Welcome to the club!

    ReplyDelete
    Replies
    1. Isn't https://codereview.chromium.org/11358134 the published change list?

      Delete