Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18999006:
Added a uploadsdk python script. (Closed)
|
Created:
7 years, 5 months ago by janicejl Modified:
7 years, 5 months ago CC:
ricow1 Visibility:
Public. |
DescriptionAdded a uploadsdk python script.
removed docs/ prefix in library_list.txt
R=efortuna@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24932
Patch Set 1 #Patch Set 2 : #
Total comments: 17
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 8
Patch Set 7 : #
Total comments: 1
Patch Set 8 : #
Total comments: 4
Patch Set 9 : #Messages
Total messages: 13 (0 generated)
Removed the docs/ previous on all the files when outputting file names in library_list.txt Python script runs docgen on the SDK, and uploads that to Cloud Storage based off the revision number.
Can we simply generalize the editor script or the tools/upload_sdk.py script to upload to a specific location as passed in as an argument instead, since we already have scripts that do very similar things here? https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.py File pkg/docgen/bin/uploadsdk.py (right): https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:10: sys.path.insert(0, '../../../tools') sys.path.append(and then again, get an absolute path) https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:12: please add a doc comment """...""" explaining how this file is used. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:13: DART = '../../../' + utils.BUILD_ROOT[utils.GuessOS()] \ absolute path is better than a relative path because on our buildbots, the checkout is on a non-local drive and also then yo can run this script while standing in any location, instead of only in the same directory as this script. Regardless, call utils.DartBinary() or utils.DartSdkBinary() instead. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:14: + '/ReleaseIA32/dart-sdk/bin/dart' +2 spaces. Also you can group this in () instead of \ See http://www.chromium.org/developers/coding-style for Python style guidelines. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:15: GSUTIL = '../../../third_party/gsutil/gsutil' please create an absoulte path instead. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:21: def ExecuteCommand(cmd): isn't this function defined in the utils file? If so, can we just call that? https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:22: # Execute a command in a subprocess. Doc comments for python are triple quotes """blah """ https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:31: def main(argv): no need to pass argument if it's not used. def main(): https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:39: return 1 throw exception here instead? No point in continuing: "raise Exception("unable to find....")" https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:53: sys.exit(main(sys.argv)) it will automatically exit after calling main. Just write this: if __name__ == '__main__': main()
I tried modifying the upload_sdk.py in tools but it can't seem to find GSUTIL the same way. I think this is because upload_sdk.py in tools is ran by the build bots. From buildbots, GSUTIL is at /b/build/scripts/slave/gsutil and docgen is looking in third_party/gsutil/gsutil Should I also try to get the docgen version of to run on build bot, and in that case probably just modify the upload_sdk.py? https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.py File pkg/docgen/bin/uploadsdk.py (right): https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:14: + '/ReleaseIA32/dart-sdk/bin/dart' On 2013/07/10 21:23:32, Emily Fortuna wrote: > +2 spaces. Also you can group this in () instead of \ > See http://www.chromium.org/developers/coding-style for Python style guidelines. Done. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:15: GSUTIL = '../../../third_party/gsutil/gsutil' On 2013/07/10 21:23:32, Emily Fortuna wrote: > please create an absoulte path instead. Done. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:21: def ExecuteCommand(cmd): On 2013/07/10 21:23:32, Emily Fortuna wrote: > isn't this function defined in the utils file? If so, can we just call that? No it is not in the utils file, it is in upload_sdk.py in the same folder as util. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:22: # Execute a command in a subprocess. On 2013/07/10 21:23:32, Emily Fortuna wrote: > Doc comments for python are triple quotes """blah """ Done. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:31: def main(argv): On 2013/07/10 21:23:32, Emily Fortuna wrote: > no need to pass argument if it's not used. > > def main(): Done. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:39: return 1 On 2013/07/10 21:23:32, Emily Fortuna wrote: > throw exception here instead? No point in continuing: > "raise Exception("unable to find....")" Done. https://codereview.chromium.org/18999006/diff/2001/pkg/docgen/bin/uploadsdk.p... pkg/docgen/bin/uploadsdk.py:53: sys.exit(main(sys.argv)) On 2013/07/10 21:23:32, Emily Fortuna wrote: > it will automatically exit after calling main. Just write this: > > > if __name__ == '__main__': > main() Done.
On 2013/07/11 00:02:08, janicejl wrote: > I tried modifying the upload_sdk.py in tools but it can't seem to find GSUTIL > the same way. I think this is because upload_sdk.py in tools is ran by the build > bots. > > From buildbots, GSUTIL is at /b/build/scripts/slave/gsutil > and docgen is looking in third_party/gsutil/gsutil > > Should I also try to get the docgen version of to run on build bot, and in that > case probably just modify the upload_sdk.py? I added ricow and kusterman in case they have an idea.
Very nice, Janice! Just a few adjustments. https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.py File pkg/docgen/bin/uploadsdk.py (right): https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.... pkg/docgen/bin/uploadsdk.py:1: #!/usr/bin/python nit: can we call this file something like upload_docgen.py or something a little bit more specific? https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.... pkg/docgen/bin/uploadsdk.py:12: import os.path this line is unnecessary. since you already import os, and you always call os.path.foo https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.... pkg/docgen/bin/uploadsdk.py:29: global GSUTIL I'd but the global declaration below the doc comment string. https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.... pkg/docgen/bin/uploadsdk.py:45: """ Execute Docgen.dart on the SDK. """ For just regular line comments, use the #. the triple quote is the equivalent of /// or /** */ in Dart
Removed the files being created by the script. https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.py File pkg/docgen/bin/uploadsdk.py (right): https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.... pkg/docgen/bin/uploadsdk.py:1: #!/usr/bin/python On 2013/07/11 18:40:37, Emily Fortuna wrote: > nit: can we call this file something like upload_docgen.py or something a little > bit more specific? Done. https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.... pkg/docgen/bin/uploadsdk.py:12: import os.path On 2013/07/11 18:40:37, Emily Fortuna wrote: > this line is unnecessary. since you already import os, and you always call > os.path.foo Done. https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.... pkg/docgen/bin/uploadsdk.py:29: global GSUTIL On 2013/07/11 18:40:37, Emily Fortuna wrote: > I'd but the global declaration below the doc comment string. Done. https://codereview.chromium.org/18999006/diff/19001/pkg/docgen/bin/uploadsdk.... pkg/docgen/bin/uploadsdk.py:45: """ Execute Docgen.dart on the SDK. """ On 2013/07/11 18:40:37, Emily Fortuna wrote: > For just regular line comments, use the #. the triple quote is the equivalent of > /// or /** */ in Dart Done.
just one question, then lgtm https://codereview.chromium.org/18999006/diff/5629499534213120/pkg/docgen/bin... File pkg/docgen/bin/upload_docgen.py (right): https://codereview.chromium.org/18999006/diff/5629499534213120/pkg/docgen/bin... pkg/docgen/bin/upload_docgen.py:54: Upload('./docs/*', GS_SITE + '/' + revision + '/') does docgen create the docs directory in whatever the current directory is that you're standing in when you invoke this script? If not, we might want to switch these to an absolute path.
PTAL. Changed quite a few of the file paths to make them relative to the scripts path.
looks good. just small adjustments https://codereview.chromium.org/18999006/diff/26001/pkg/docgen/bin/upload_doc... File pkg/docgen/bin/upload_docgen.py (right): https://codereview.chromium.org/18999006/diff/26001/pkg/docgen/bin/upload_doc... pkg/docgen/bin/upload_docgen.py:15: sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), here's a little trick that will make these lines less long. You can say "from os.path import join, dirname, abspath" and then in your code you can just call the function lke this: sys.path.append(abspath(join(dirname(__file__))) which is nice when you have many calls to os.path functions. https://codereview.chromium.org/18999006/diff/26001/pkg/docgen/bin/upload_doc... pkg/docgen/bin/upload_docgen.py:26: print DART debug code?
https://codereview.chromium.org/18999006/diff/26001/pkg/docgen/bin/upload_doc... File pkg/docgen/bin/upload_docgen.py (right): https://codereview.chromium.org/18999006/diff/26001/pkg/docgen/bin/upload_doc... pkg/docgen/bin/upload_docgen.py:15: sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), On 2013/07/11 22:41:24, Emily Fortuna wrote: > here's a little trick that will make these lines less long. You can say > "from os.path import join, dirname, abspath" > and then in your code you can just call the function lke this: > sys.path.append(abspath(join(dirname(__file__))) > which is nice when you have many calls to os.path functions. Done. https://codereview.chromium.org/18999006/diff/26001/pkg/docgen/bin/upload_doc... pkg/docgen/bin/upload_docgen.py:26: print DART On 2013/07/11 22:41:24, Emily Fortuna wrote: > debug code? Done.
lgtm!
Message was sent while issue was closed.
Committed patchset #9 manually as r24932 (presubmit successful).
Message was sent while issue was closed.
On 2013/07/11 18:08:54, Andrei Mouravski wrote: > On 2013/07/11 00:02:08, janicejl wrote: > > I tried modifying the upload_sdk.py in tools but it can't seem to find GSUTIL > > the same way. I think this is because upload_sdk.py in tools is ran by the > build > > bots. > > > > From buildbots, GSUTIL is at /b/build/scripts/slave/gsutil > > and docgen is looking in third_party/gsutil/gsutil > > > > Should I also try to get the docgen version of to run on build bot, and in > that > > case probably just modify the upload_sdk.py? > > I added ricow and kusterman in case they have an idea. I came across this as well. We should've just renamed the method in utils.py to "utils.GetGSUtilPath()' and changed it to search at both places. (Sorry that I'm commenting so late -- I was a bit busy with other things.) |
