Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11343027:
Make the editor build callable from tools/build.py (Closed)
|
Created:
8 years, 1 month ago by devoncarew Modified:
7 years, 11 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMake the editor build callable from tools/build.py
Committed: https://code.google.com/p/dart/source/detail?r=16979
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Patch Set 5 : #
Total comments: 30
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Messages
Total messages: 16 (0 generated)
https://chromiumcodereview.appspot.com/11343027/diff/14/dart.gyp File dart.gyp (right): https://chromiumcodereview.appspot.com/11343027/diff/14/dart.gyp#newcode127 dart.gyp:127: #'<!@(["python", "tools/list_files.py", "\\.java$", "editor/tools/plugins"])', Currently we always run the build_editor_py action when the editor target is specified. W/ the above line commented in, gclient runhooks works, but then the editor target fails (from tools/build.py). I suspect that we have way too many java input files. Any ideas are appreciated!
Hi Devon, This is a great start, but please don't submit this until we are sure the build is incremental. We currently have two problems: long file lists in this CL, and a change Ivan made recently that touches the version file each time we run gclient runhooks. I believe that we can and must fix these problems before submitting this CL, as the alternative is that build times go up for all engineers on the team which translates into lost productivity. As far as I recall, I solved a similar problem with long file lists in the dartc build. I'll send more info in the next mail.
Found it! This is from deps/all.deps/DEPS:
{
"pattern": ".",
"action": ["python", "dart/tools/generate_projects.py", "compiler"],
},
This script generates .gypi and .xml files which list all the dependencies on
dartc. The .xml files are included by build.xml and the .gypi files are included
in dart/compiler/dart-compiler.gyp and ensure that GYP has precise dependencies
to avoid invoking ant if nothing has changed.
You should be able to extend/generalize this to the editor build.
Cheers,
Peter
> This is a great start, but please don't submit this until we are sure the build > is incremental. Of course - I'll wait for the lgtm > We currently have two problems: long file lists in this CL, and a change Ivan > made recently that touches the version file each time we run gclient runhooks. I > believe that we can and must fix these problems before submitting this CL, as > the alternative is that build times go up for all engineers on the team which > translates into lost productivity. Even when the editor target becomes incremental, it's still a 2-3 minute build. About 97% of that time is us handing off the the PDE build process. That's mostly a black box from our point of view. So I don't think that people will want to be always building the editor - more of a "I made a change, let me build and test it" thing. > dart/tools/generate_projects.py Thanks for the tip! I'll dig into this and ping you if I have any questions.
So, I added an entry to the DEPS file and created an editor/build/generate_sources.py script which generates a 2600 line editor_sources.gypi file. That's then included into the dart.gyp file and used as an input into the editor target. That's the good news. The bad news is that I'm seeing the same behavior as before - runhooks works, but the build fails when it gets to the editor target. Commenting out the '<@(editor_sources)' line in dart.gyp fixes that. So I think we're running into an intrinsic file limit in the input list. I'm going to look into creating a new target that the editor depends on, something like editor_timestamp. This will create a timestamp file that is the same time as the newest editor input file, and the editor will use that timestamp file as an input. So this editor_timestamp target will be fast but not incremental; the editor target itself would be incremental.
On 2012/11/09 19:01:48, devoncarew wrote: > So, I added an entry to the DEPS file and created an > editor/build/generate_sources.py script which generates a 2600 line > editor_sources.gypi file. That's then included into the dart.gyp file and used > as an input into the editor target. > > That's the good news. The bad news is that I'm seeing the same behavior as > before - runhooks works, but the build fails when it gets to the editor target. > Commenting out the '<@(editor_sources)' line in dart.gyp fixes that. So I think > we're running into an intrinsic file limit in the input list. > > I'm going to look into creating a new target that the editor depends on, > something like editor_timestamp. This will create a timestamp file that is the > same time as the newest editor input file, and the editor will use that > timestamp file as an input. So this editor_timestamp target will be fast but not > incremental; the editor target itself would be incremental. I don't think that would work. Could you upload editor/build/generate_sources.py and your changes to GYP that doesn't work. I have seen this problem before and fixed it.
> I don't think that would work. Could you upload
> editor/build/generate_sources.py and your changes to GYP that doesn't work. I
> have seen this problem before and fixed it.
Everything should be in the CL now, except for my changes to all.deps/DEPS. I
added the following lines of code to the hooks section:
{
"pattern": ".",
"action": ["python", "dart/editor/build/generate_sources.py"],
},
The Ant and Java changes look good, but I can't comment on the other changes.
PTAL - this has been reswizzled to not modify the dart.gyp or all.deps/DEPS file. So this is not called by the tools/build.py script - that's work for another day.
LGTM with comments... https://codereview.chromium.org/11343027/diff/17001/editor/tools/features/com... File editor/tools/features/com.google.dart.tools.deploy.feature_releng/build_rcp.xml (right): https://codereview.chromium.org/11343027/diff/17001/editor/tools/features/com... editor/tools/features/com.google.dart.tools.deploy.feature_releng/build_rcp.xml:186: <!-- delete dir="${samples.out.dir}"/ --> Lets leave this in there for now... unless its causing major problems on Windows https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py File tools/create_editor.py (right): https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:61: DeleteDirContents(OUTPUT) shutil.rmtree(OUTPUT, True) instead of the above line?
https://codereview.chromium.org/11343027/diff/17001/editor/tools/features/com... File editor/tools/features/com.google.dart.tools.deploy.feature_releng/build_rcp.xml (right): https://codereview.chromium.org/11343027/diff/17001/editor/tools/features/com... editor/tools/features/com.google.dart.tools.deploy.feature_releng/build_rcp.xml:186: <!-- delete dir="${samples.out.dir}"/ --> On 2013/01/07 23:07:54, danrubel wrote: > Lets leave this in there for now... unless its causing major problems on Windows Done. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py File tools/create_editor.py (right): https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:61: DeleteDirContents(OUTPUT) On 2013/01/07 23:07:54, danrubel wrote: > shutil.rmtree(OUTPUT, True) instead of the above line? Done.
Thanks a lot for getting the ball moving on this. This does not seem to eliminate all the "hassle" of building, I assume there is still a number of steps that one needs to perform before one can build the editor, e.g., copy around <username>.properties files in build settings, setting eclipse home. Additionally, if I need to run the tests I need to build these as well right? Can we add functionality for that as well? https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py File tools/create_editor.py (right): https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:3: # Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file Since this is a new file - 2013 https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:9: # Usage: ./tools/build.py editor I do not think this is valid with the current cl (but I am actually more than fine with us adding that in a follow up) https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:12: import glob could we order these alphabetically https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:36: def Main(argv): move Main method down right above if __name__ == '__main__': sys.exit(Main(sys.argv)) https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:43: OUTPUT = argv[1] why not use utils.GetBuildDir (see below, we also need to import this) and then create a editor directory in there https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:45: OUTPUT = os.path.abspath(OUTPUT) Verify that this is a valid directory (or even better, use utils.GetBuildDir https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:47: osName = utils.GuessOS() can you do this, I think you need to import our utils lib for this to work https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:48: mode = ('release', 'debug')['Debug' in OUTPUT] could we add a --mode option instead, defaulting to Debug, this is standard for our normal build script https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:49: arch = ('ia32', 'x64')['X64' in OUTPUT] same as for --mode https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:55: if GetShouldClobber(): I think this still belongs in the annotated steps script, not here. This is buildbot specific functionality, but I see that there are some additional stuff being deleted, so maybe we need to update the annotated steps to actually do that? https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:61: DeleteDirContents(OUTPUT) Could we add a TODO here to say to remove when we make this incremental https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:61: DeleteDirContents(OUTPUT) I would probably also print that I am deleting the directory, seems like relevant info https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:63: # macosx, cocoa, x86 & macosx, cocoa, x86_64 I have a hard time reading these comments https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:143: return join(os.path.expanduser('~'), '.editor.download.cache') Any reason why not to put this into the output directory?
Hey Rico, I think I addressed or responded to all of your issues. > This does not seem to eliminate all the "hassle" of building, I assume there is > still a number of steps that one needs to perform before one can build the > editor, e.g., copy around <username>.properties files in build settings, setting > eclipse home. No, this is a one-stop-shop for building the editor. It handles all the args that the build_rcp.xml editor script wants. All the user needs to do is run ./tools/create_editor.py xcodebuild/ReleaseIA32/editor. No properties files necessary, no eclipse home env variables required ;) > Additionally, if I need to run the tests I need to build these as well right? > Can we add functionality for that as well? I definitely want to get this integrated into tools/build.py and into the test framework, but I think that's work for future CLs. Our current test setup builds the tests, then runs them. I'd want the build step to build both the RCP and the tests, and the test step to just run them. It's all handled right now in editor/tools/features/com.google.dart.tools.tests.feature_releng/buildTests.xml. It'll take a lot of work to separate that all out, and I want to stage that over several CLs. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py File tools/create_editor.py (right): https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:3: # Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file On 2013/01/08 07:08:26, ricow1 wrote: > Since this is a new file - 2013 Done. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:9: # Usage: ./tools/build.py editor On 2013/01/08 07:08:26, ricow1 wrote: > I do not think this is valid with the current cl (but I am actually more than > fine with us adding that in a follow up) I'll add a TODO: here saying that it doesn't work that way yet, but that's our goal. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:12: import glob On 2013/01/08 07:08:26, ricow1 wrote: > could we order these alphabetically I think they are already. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:36: def Main(argv): On 2013/01/08 07:08:26, ricow1 wrote: > move Main method down right above > if __name__ == '__main__': > sys.exit(Main(sys.argv)) Done. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:47: osName = utils.GuessOS() On 2013/01/08 07:08:26, ricow1 wrote: > can you do this, I think you need to import our utils lib for this to work We are importing it... https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:48: mode = ('release', 'debug')['Debug' in OUTPUT] On 2013/01/08 07:08:26, ricow1 wrote: > could we add a --mode option instead, defaulting to Debug, this is standard for > our normal build script My goal is for this to be callable from our normal build script. and for it to determine the mode and arch options automatically based on the path. My template is how create_sdk is called from the dart.gyp file. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:55: if GetShouldClobber(): On 2013/01/08 07:08:26, ricow1 wrote: > I think this still belongs in the annotated steps script, not here. This is > buildbot specific functionality, but I see that there are some additional stuff > being deleted, so maybe we need to update the annotated steps to actually do > that? Ah, from looking at tools/clean_output_directory.py, it looks like it does everything we need on a clobber. I'll remove this if statement. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:61: DeleteDirContents(OUTPUT) On 2013/01/08 07:08:26, ricow1 wrote: > I would probably also print that I am deleting the directory, seems like > relevant info I added a printout for when we clean the out directory. Even when the build becomes incremental, we'll still want to clean the output directory before a build. The eclipse build artifacts are named based on timestamps and svn rev ids. If we don't clean before a build we'll have lots of plugins accumulate very fast. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:63: # macosx, cocoa, x86 & macosx, cocoa, x86_64 On 2013/01/08 07:08:26, ricow1 wrote: > I have a hard time reading these comments I'll add some explaination. https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:143: return join(os.path.expanduser('~'), '.editor.download.cache') On 2013/01/08 07:08:26, ricow1 wrote: > Any reason why not to put this into the output directory? No, that sounds reasonable - done.
LGTM https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py File tools/create_editor.py (right): https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:48: mode = ('release', 'debug')['Debug' in OUTPUT] On 2013/01/09 21:08:09, devoncarew wrote: > On 2013/01/08 07:08:26, ricow1 wrote: > > could we add a --mode option instead, defaulting to Debug, this is standard > for > > our normal build script > > My goal is for this to be callable from our normal build script. and for it to > determine the mode and arch options automatically based on the path. My template > is how create_sdk is called from the dart.gyp file. My only concern here is: when we change the name of the output directory this is going to break (I would really like us to only have to change stuff like that in one place). https://codereview.chromium.org/11343027/diff/28001/tools/create_editor.py File tools/create_editor.py (right): https://codereview.chromium.org/11343027/diff/28001/tools/create_editor.py#ne... tools/create_editor.py:72: if os.path.isabs(root): can you just return os.path.abspath(root) I assume that will check if it is absolute and just return it in that case
https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py File tools/create_editor.py (right): https://codereview.chromium.org/11343027/diff/17001/tools/create_editor.py#ne... tools/create_editor.py:48: mode = ('release', 'debug')['Debug' in OUTPUT] > My only concern here is: when we change the name of the output directory this is > going to break (I would really like us to only have to change stuff like that in > one place). Gotcha, and that makes perfect sense to me. I'll keep the output path name scrapping, but add --mode and --arch flags which will be used if passed in. I'll also add a TODO: that we'll want to revisit this when the script is callable from dart.gyp. https://codereview.chromium.org/11343027/diff/28001/tools/create_editor.py File tools/create_editor.py (right): https://codereview.chromium.org/11343027/diff/28001/tools/create_editor.py#ne... tools/create_editor.py:72: if os.path.isabs(root): On 2013/01/10 13:23:12, ricow1 wrote: > can you just return os.path.abspath(root) > I assume that will check if it is absolute and just return it in that case I tried it and this works fine - removed the isabs() check. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
