I want to better support building mobile/android with Eclipse, and step one is to ship (clones of) romaxa's excellent Perl scripts. bnicholson has also done work improving these scripts. romaxa's repo: http://hg.mozilla.org/users/romaxa_gmail.com/eclipse_mobile bnicholson's repo: https://github.com/thebnich/eclipse-fennec

This ports most of romaxa's work to Python. It does not include bnicholson's java.in handling. A few tweaks are still needed to make this work smoothly.

No longer need to run `mach` in virtualenv.

This includes a build script for mach plus IDE dirs, which gives us support for Eclipse and IntelliJ. This is still a WIP, but we should land this ASAP so people can take advantage of IDE features and give feedback. High-level breakdown: * Requires a full build (./mach build) before this script is run * projectify is run once by the user to generate project files and symlinks * src, res, and thirdparty symlinks point directly to source in tree * libs and assets symlinks point to those objdir/dist/fennec * projectify is also run at each build step in the IDE to generate preprocessed resources * includes *.java.in files, strings.xml.in, R.java, AndroidManifest.xml.in * Projects can have only one res/ dir, but we want to include outside resources (such as branding files and the preprocessed strings.xml) in our res folder. To work around this, projectify creates a nested project called FennecPreprocessedResources, which is a dependency for the primary project. This is an Android library project containing just these external resources. Usage: ./mach projectify -i [eclipse|intellij] -w <IDE workspace dir> Importing in Eclipse: * File > Import... > General > Existing Projects into Workspace * Select project dir as root directory * Make sure "Search for nested projects" is checked * Make sure both Fennec and FennecPreprocessedResources are selected projects * Click Finish Importing in IntelliJ: * At welcome dialog, choose Open Project and choose project dir * File > Project Structure... * Choose Project on left, then specify your Android SDK in Project SDK (creating one if necessary) * Click OK Much of mach_commands.py was written by Nick, so this may need more eyes (gps?).

We can make Eclipse run stuff before the Java builder if that's of interest. For more information, see: http://www.eclipse.org/articles/Article-Builders/builders.html It's also an option to bring in the CDT as a requirement if we want some Makefile features.

Comment on attachment 819252 [details] [diff] [review] Implement `mach projectify` Review of attachment 819252 [details] [diff] [review]: ----------------------------------------------------------------- This is a great first cut! I've asked for lots of cutting and some nits, and some investigation before a second version. I've filed some additional moz.build bugs and think we should do some preliminary work to figure out if there are easy wins, and how much they would help this ticket. f+! ::: mobile/android/mach_commands.py @@ +43,5 @@ > + FileAvoidWrite, > +) > + > + > +class ProjectCreator(MozbuildObject): Please include a few lines talking about the envisioned project structure (with the generated sub-project), and what needs to be implemented for this to work. @@ +81,5 @@ > + def warn(self, *args, **kwargs): > + LOG_TAG = 'create_ide_projects' > + self.log(logging.WARN, LOG_TAG, *args, **kwargs) > + > + # IDE-specific project creation Document the two needed overrides in the class comment. @@ +100,5 @@ > + > + # projectify-specific defines > + defines['IDE_PROJECT_NAME'] = self.project_name > + defines['IDE_WORKSPACE_DIR'] = self.workspace_directory > + defines['MOZCONFIG_PATH'] = os.environ['MOZCONFIG'] For me, this fails. We should be able to get this from mach somehow. Can you investigate? @@ +102,5 @@ > + defines['IDE_PROJECT_NAME'] = self.project_name > + defines['IDE_WORKSPACE_DIR'] = self.workspace_directory > + defines['MOZCONFIG_PATH'] = os.environ['MOZCONFIG'] > + > + # TODO: We should transition these to config.status defines ASAP. We are *so close* to being able to do this from moz.build. DEFINES is supported in moz.build, but there are few tricky bits: ANDROID_VERSION_CODE, STRINGSPATH, SYNCSTRINGSPATH, MOZ_APP_BUILDID, MOZ_ANDROID_SHARED_ACCOUNT_TYPE, etc. I've filed Bug 929296 to track this. Update the comment to reference that ticket. @@ +116,5 @@ > + defines['MOZ_ANDROID_SHARED_FXACCOUNT_TYPE'] = defines['ANDROID_PACKAGE_NAME'] + "_account" > + defines['MOZ_APP_ABI'] = defines['TARGET_XPCOM_ABI'] > + defines['MANGLED_ANDROID_PACKAGE_NAME'] = defines['ANDROID_PACKAGE_NAME'].replace('fennec', 'f3nn3c') > + defines['OBJDIR'] = os.path.join(self.topobjdir, "mobile", "android", "base") > + defines['STRINGSPATH'] = os.path.join(self.topsrcdir, "mobile", "android", "base", "locales", "en-US", "android_strings.dtd") Shouldn't these by relative to MOZ_BRANDING_DIRECTORY? Nope. Oh God, it's interacting with l10n repack. If this works, it's good enough for me for now. @@ +124,5 @@ > + > + @property > + def mozbuild_sandbox(self): > + if self._mozbuild_sandbox is None: > + path = os.path.join(self.topsrcdir, 'mobile/android/base/moz.build') Looking ahead to building projects for other pieces (like robocop, background junit3 tests, and browser junit 3 tests), let's include the base moz.build as a parameter (which can be hard-coded for now). As we move APK generation to moz.build, this will make more sense. See also Bug 929298, where we will define all these Android APK generating things. @@ +156,5 @@ > + self._ensureParentDir(output_filename) > + > + # Avoid writing unchanged files. This trades memory (since > + # output is buffered) to save disk access (refreshes and > + # recompiles in Eclipse). $IDE. @@ +182,5 @@ > + self._run_command_in_objdir(args=args, line_handler=_lh) > + > + return lines > + > + def _order_symlinks(self, d): We're not symlinking all the source files anymore, right? If that's the case, let's kill this. @@ +229,5 @@ > + > + def _update_or_create_symlink(self, src, dst): > + r''' > + > + Make `dst` point to `src` but don't always re-create links, Likewise, we're not symlinking all that much, right? Can we kill this? And maybe that would let us get rid of _shorten above, which isn't that valuable. @@ +276,5 @@ > + ordered = self._order_symlinks(assocs) > + total = len(ordered) > + count = 0 > + for (src, dst) in ordered: > + self._ensureParentDir(dst) I see styles have clashed. Let's replace CamelCase with _camel_case. @@ +284,5 @@ > + self.warn({'count': count, 'total': total}, > + 'Created {count} of {total} symlinks.') > + > + def _get_package(self, fileobj): > + for line in fileobj.readlines(): Do we still need this? We should address the root issue rather than working around it if at all possible. Bug 890586? @@ +289,5 @@ > + line = line.strip() > + if line.startswith('package'): > + break > + # TODO: fix this hack > + if line == "#include GeckoView.java.frag": This is no longer necessary -- all GeckoView.java.in files are in widget/. \o/ @@ +301,5 @@ > + package = package.replace('@ANDROID_PACKAGE_NAME@', self.substs['ANDROID_PACKAGE_NAME']) > + > + return package > + > + def _files_from_manifest(self, mn, dst_dir): I just landed Bug 904299; the Android Services manifests are no longer in the tree. If we need just the services resources, we can get them from mobile/android/base/android-services.mozbuild -- but I'd rather not use them at all. Why do we need the service manifests in particular? @@ +315,5 @@ > + > + return d > + > + # TODO: get this from Makefile.in. > + def _branding_files(self): The destinations are in ANDROID_GENERATED_RESFILES but the sources aren't :( The approach below is pretty awful, and I really don't want to land this duplicated nastiness. For now, make this less general but more clear and just hard code the 4 icon files. We'll need to think about installing these by manifests (Bug 928151). @@ +357,5 @@ > + def _java_in_preprocess_files(self): > + d = {} > + for fn in self._find(True, "mobile/android/base", "*.java.in"): > + bn = os.path.basename(fn) > + if bn in ['CrashReporter.java'] and not self.defines('MOZ_CRASHREPORTER').strip(): Ugh. We can do this properly once we have JAVA_JAR_TARGETS in moz.build (Bug 925185). @@ +361,5 @@ > + if bn in ['CrashReporter.java'] and not self.defines('MOZ_CRASHREPORTER').strip(): > + continue > + > + #TODO: add back robotium tests > + if os.path.basename(os.path.dirname(fn)) == "tests": Move this test before the CrashReporter test. And please file for building an IDE project for Robocop tests. @@ +375,5 @@ > + return d > + > + def _links(self): > + symlinks = { > + os.path.join(self.topobjdir, "dist", "fennec", "assets") : These are an interesting case. We'd like to get this kind of extra information from moz.build, but it's truly meta data and doesn't define the build (yet). Roll on. @@ +378,5 @@ > + symlinks = { > + os.path.join(self.topobjdir, "dist", "fennec", "assets") : > + os.path.join(self.project_directory, "assets"), > + > + os.path.join(self.topobjdir, "dist", "fennec", "lib", "armeabi-v7a") : Hmm, this brittle. Do we have build architecture info available in moz.build or config.status? Please investigate. My guess is no. This would be part of Bug 929296. @@ +391,5 @@ > + os.path.join(self.topsrcdir, "mobile", "android", "thirdparty") : > + os.path.join(self.project_directory, "thirdparty"), > + > + self.pp.context['ANDROID_COMPAT_LIB'] : > + os.path.join(self.project_directory, "libs", os.path.basename(self.pp.context['ANDROID_COMPAT_LIB'])) I thought Eclipse had a special way of defining this? I'm surprised it's supposed to be dumped in libs/. @@ +427,5 @@ > + dst = os.path.join(self.preprocessed_resources_project_directory, "res", "values", "strings.xml") > + self._preprocess_files({ src : dst }) > + > + def _generate_r_java(self): > + aapt = self.pp.context['AAPT'] Urgh, this build system duplication is frustrating. We would get this from Bug 919563... @@ +447,5 @@ > + self._create_preprocessed_resources_project_files() > + self._create_links() > + > + self._create_preprocessed_files() > + self._generate_r_java() This will create R.java fresh every build, correct? That's not good for fast builds. Perhaps we should generate R.java somewhere temporary and only copy it over when it changes, to prevent recompiles. Thoughts? If we used the build system to do this, we wouldn't have spurious updates. But we'd have problems with the preprocessed library project. Sigh. @@ +463,5 @@ > + "dotproject": ".project", > + "project.properties": "project.properties", > + } > + for fn in fns: > + self._preprocess(os.path.join(self.template_directory, "PreprocessedResources", fn), Just build the dict and do all the _preprocess calls uniformly. @@ +488,5 @@ > + bn = os.path.basename(fn) > + dst = os.path.join(self.project_directory, ".externalToolBuilders", bn) > + self._preprocess(fn, dst) > + > + self._preprocess(os.path.join(self.template_directory, "org.eclipse.jdt.core.prefs"), Just build the dict above once, and do all the _preprocess calls uniformly. Might be worth making _preprocess take a dict. @@ +504,5 @@ > + "PreprocessedResources.iml": self.project_name + "PreprocessedResources.iml", > + } > + for fn in fns: > + self._preprocess(os.path.join(self.template_directory, "PreprocessedResources", fn), > + os.path.join(self.preprocessed_resources_project_directory, fns[fn])) I have a feeling that we'll someday include more than just resources in the Preprocessed project. Let's just call it Preprocessed and leave that door open. @@ +527,5 @@ > + "workspace.xml": "workspace.xml", > + } > + > + for fn in fns: > + self._preprocess(os.path.join(self.template_directory, fn), ditto for just one _preprocess call. @@ +528,5 @@ > + } > + > + for fn in fns: > + self._preprocess(os.path.join(self.template_directory, fn), > + os.path.join(self.project_directory, ".idea", fns[fn])) Might want to consider changing the abstraction -- make the default implementation _create* implementations call _preprocess(_project_files_dict) and _preprocessed_project_files_dict, for example. Both implementations are very similar. In fact, each creator is just two dicts, but I don't want to get rid of the abstraction entirely :) @@ +544,5 @@ > + action='store_true', > + help='do not create project files.') > + def projectify(self, **params): > + ide_list = {} > + self._register_creator(ide_list, EclipseProjectCreator) No need for this trickiness. |ide_list| is actually a dict, so just call it |creators|, hard-code it, and get rid of _register_creator.

Comment on attachment 819252 [details] [diff] [review] Implement `mach projectify` Review of attachment 819252 [details] [diff] [review]: ----------------------------------------------------------------- The mach_commands.py file is quite large. It is a good practice to keep as much "business logic" as possible in importable modules rather than inside mach_commands.py. This facilitates testing as well as faster execution times (since the mach driver needs to import mach_commands.py files at run time). Also, we need to talk about how we want to facilitate project generation. An argument can be made that project generation should be built into config.status. i.e. you create a separate build system "backend" that outputs project files. Keep in mind a backend doesn't actually need to build things - it's just a consumer of build config metadata. Early thoughts were that we'd offer a mach command to regenerate the build backend and this mach command could be used to generate Eclipse, Visual Studio, etc project files in addition to make files, etc. I can't remember what bug I had a patch in...

(In reply to Nick Alexander :nalexander from comment #5) > @@ +100,5 @@ > > + > > + # projectify-specific defines > > + defines['IDE_PROJECT_NAME'] = self.project_name > > + defines['IDE_WORKSPACE_DIR'] = self.workspace_directory > > + defines['MOZCONFIG_PATH'] = os.environ['MOZCONFIG'] > > For me, this fails. We should be able to get this from mach somehow. Can > you investigate? Indeed, self.mozconfig['path'] does the trick. > @@ +124,5 @@ > > + > > + @property > > + def mozbuild_sandbox(self): > > + if self._mozbuild_sandbox is None: > > + path = os.path.join(self.topsrcdir, 'mobile/android/base/moz.build') > > Looking ahead to building projects for other pieces (like robocop, > background junit3 tests, and browser junit 3 tests), let's include the base > moz.build as a parameter (which can be hard-coded for now). As we move APK > generation to moz.build, this will make more sense. See also Bug 929298, > where we will define all these Android APK generating things. We don't actually use mozbuild_sandbox anywhere now; I just kept it around assuming we'd want it in the future. I changed mozbuild to be a parameter which defaults to 'mobile/android/base/moz.build'. > @@ +229,5 @@ > > + > > + def _update_or_create_symlink(self, src, dst): > > + r''' > > + > > + Make `dst` point to `src` but don't always re-create links, > > Likewise, we're not symlinking all that much, right? Can we kill this? And > maybe that would let us get rid of _shorten above, which isn't that valuable. We don't symlink source files anymore, but we do still symlink the base project dirs (src, res, libs, assets, etc). I think we still want to keep this update-and-replace behavior in case projectify is re-run on an existing project. This will be particularly useful if the symlinks ever change: we don't want users to have to delete their entire project just to update them. (I removed _shorten though, which was just used for logging.) > @@ +276,5 @@ > > + ordered = self._order_symlinks(assocs) > > + total = len(ordered) > > + count = 0 > > + for (src, dst) in ordered: > > + self._ensureParentDir(dst) > > I see styles have clashed. Let's replace CamelCase with _camel_case. I found that we're already importing mozbuild.util.ensureParentDir but not using it, so I removed our local _ensureParentDir altogether and replaced it with mozbuild.util's version. > @@ +284,5 @@ > > + self.warn({'count': count, 'total': total}, > > + 'Created {count} of {total} symlinks.') > > + > > + def _get_package(self, fileobj): > > + for line in fileobj.readlines(): > > Do we still need this? We should address the root issue rather than working > around it if at all possible. Bug 890586? The problem is that we intermingle both org.mozilla.gecko and org.mozilla.fennec_X package sources. Given just a path, there was no way to determine which package the file uses. I filed bug 929865 to fix this by moving these to a separate directory and updated this method accordingly. > @@ +301,5 @@ > > + package = package.replace('@ANDROID_PACKAGE_NAME@', self.substs['ANDROID_PACKAGE_NAME']) > > + > > + return package > > + > > + def _files_from_manifest(self, mn, dst_dir): > > I just landed Bug 904299; the Android Services manifests are no longer in > the tree. If we need just the services resources, we can get them from > mobile/android/base/android-services.mozbuild -- but I'd rather not use them > at all. Why do we need the service manifests in particular? I don't think we do need them. Removed. > @@ +315,5 @@ > > + > > + return d > > + > > + # TODO: get this from Makefile.in. > > + def _branding_files(self): > > The destinations are in ANDROID_GENERATED_RESFILES but the sources aren't :( > The approach below is pretty awful, and I really don't want to land this > duplicated nastiness. For now, make this less general but more clear and > just hard code the 4 icon files. We'll need to think about installing these > by manifests (Bug 928151). I'm not really sure what the story is with these branding files; removing them, there's no project breakage. For now, I've just removed _branding_files altogether. > @@ +361,5 @@ > > + if bn in ['CrashReporter.java'] and not self.defines('MOZ_CRASHREPORTER').strip(): > > + continue > > + > > + #TODO: add back robotium tests > > + if os.path.basename(os.path.dirname(fn)) == "tests": > > Move this test before the CrashReporter test. And please file for building > an IDE project for Robocop tests. Filed bug 929654. > @@ +378,5 @@ > > + symlinks = { > > + os.path.join(self.topobjdir, "dist", "fennec", "assets") : > > + os.path.join(self.project_directory, "assets"), > > + > > + os.path.join(self.topobjdir, "dist", "fennec", "lib", "armeabi-v7a") : > > Hmm, this brittle. Do we have build architecture info available in > moz.build or config.status? Please investigate. My guess is no. This > would be part of Bug 929296. Looks like config.status already has ANDROID_CPU_ARCH \o/ > > @@ +391,5 @@ > > + os.path.join(self.topsrcdir, "mobile", "android", "thirdparty") : > > + os.path.join(self.project_directory, "thirdparty"), > > + > > + self.pp.context['ANDROID_COMPAT_LIB'] : > > + os.path.join(self.project_directory, "libs", os.path.basename(self.pp.context['ANDROID_COMPAT_LIB'])) > > I thought Eclipse had a special way of defining this? I'm surprised it's > supposed to be dumped in libs/. That *is* Eclipse's special way of defining this :) For whatever reason, Eclipse requires that this be in libs. I've tried referencing the jar directly from the Android SDK directory, but that gives errors when trying to launch. Also, when I create a new Android project in Eclipse, that's where it puts this jar. Doesn't really make sense to me, especially since that's where the native libs are also supposed to go. > @@ +447,5 @@ > > + self._create_preprocessed_resources_project_files() > > + self._create_links() > > + > > + self._create_preprocessed_files() > > + self._generate_r_java() > > This will create R.java fresh every build, correct? That's not good for > fast builds. Perhaps we should generate R.java somewhere temporary and only > copy it over when it changes, to prevent recompiles. Thoughts? If we used > the build system to do this, we wouldn't have spurious updates. But we'd > have problems with the preprocessed library project. Sigh. In Eclipse, it's actually much more frequent than that. projectify gets run every time *any* file changes, generating a new R.java and preprocessing all *.java.in files. It sounds like a lot, and it is, but since Eclipse does incremental builds automatically, I'm not sure how we could improve this. Without it, I was seeing errors when adding/removing strings, and the only way to fix them was to clean/rebuild the project. The good news is that projectify finishes pretty quickly, and adding this builder doesn't result in any noticeable lag -- at least from my limited testing in Eclipse. > @@ +504,5 @@ > > + "PreprocessedResources.iml": self.project_name + "PreprocessedResources.iml", > > + } > > + for fn in fns: > > + self._preprocess(os.path.join(self.template_directory, "PreprocessedResources", fn), > > + os.path.join(self.preprocessed_resources_project_directory, fns[fn])) > > I have a feeling that we'll someday include more than just resources in the > Preprocessed project. Let's just call it Preprocessed and leave that door > open. I made this change, though I can't imagine we'd be putting anything other than resources in this project. At first I wanted to move the preprocessed/ directory to Preprocessed/src/, but preprocessed sources can't be outside of the primary project since they rely on all of the classes there. > Might want to consider changing the abstraction -- make the default > implementation _create* implementations call > _preprocess(_project_files_dict) and _preprocessed_project_files_dict, for > example. Both implementations are very similar. In fact, each creator is > just two dicts, but I don't want to get rid of the abstraction entirely :) Good idea -- I had actually meant to do that before posting this patch but forgot.

Updated _get_package to work with the new approach in bug 929865.

Comment on attachment 821244 [details] [diff] [review] Implement `mach projectify`, v3 Review of attachment 821244 [details] [diff] [review]: ----------------------------------------------------------------- This is looking really great. Major request is to do what gps requested: let's add mobile/android/python, shunt the bulk of mach_commands.py into mobile/android/python/projects.py, and import it (on demand) in the body of the mach command. Only other issue is whether we have any good ideas for not running |mach projectify|, and whether we want to not land this until we have movement there. I'm inclined to let this ride, see how it fares in the wild, and iterate as we gain understanding and land build system improvements. We should collaborate on a mobile-firefox-dev message and a blogpost giving a little tutorial on how to use this and how it works. ::: mobile/android/mach_commands.py @@ +40,5 @@ > + > + Creates a generic project structure compatible with IDEs. > + > + The following is an outline of the created structure: > + assets/ symlinked to <obdjir>/dist/fennec/assets This is great. @@ +48,5 @@ > + thirdparty/ symlinked to <srcdir>/mobile/android/thirdparty > + FennecPreprocessed/ generated by projectify > + preprocessed/ generated by projectify > + > + FennecPreprocessed is a nested Android library project used solely nit: renamed |Preprocessed|. @@ +62,5 @@ > + IDE-specific project creator: > + _project_files > + dict of the Fennec project files > + _preprocessed_project_files > + dict of the FennecPreprocessed subproject files nit: renamed. @@ +101,5 @@ > + > + # IDE-specific Preprocessed project creation > + @property > + def _preprocessed_project_files(self): > + raise Exception("Project creator must provide _preprocessed_project_files dict") nit: NotImplementedError @@ +152,5 @@ > + self.pp.context.update(defines) > + > + def _preprocess(self, input_filename, output_filename): > + ''' > + nit: no extra newlines before or after triple quotes. @@ +169,5 @@ > + self.pp.out = fo > + with open(input_filename, "rt") as fi: > + self.pp.do_include(fi) > + > + def _preprocess_files(self, assocs): |assocs| is really a dict, right? (Coming from lisp, I expect assocs to be a list of pairs.) @@ +476,5 @@ > + if not ide or not ide in creators: > + raise Exception('IDE name must be provided. Valid IDEs: ' + ', '.join(creators.keys())) > + if not workspace: > + raise Exception('IDE workspace directory must be provided.') > + project = params.pop('project') nit: prevent foolery like --project=''

A few additional fixes: * Carry over the project name to build.xml * After bug 929865, IntelliJ is broken as it does not support launching activity-alias as the default activity (to be fixed in IntelliJ 13: http://youtrack.jetbrains.com/issue/IDEA-91552). Workaround is to manually specify ".App". * Remove preprocessed dir at every build to prevent preprocessed garbage from being left behind after deletions/renames.

Bug 929865 is blocking this. But once that's fixed, this should be ready!

(In reply to Brian Nicholson (:bnicholson) from comment #11) > Bug 929865 is blocking this. But once that's fixed, this should be ready! We need to figure out what's happening with the android:sharedUserId first. Without fixing that, Sync won't work.

(In reply to Brian Nicholson (:bnicholson) from comment #11) > Bug 929865 is blocking this. But once that's fixed, this should be ready! I'd like to suggest that rather than blocking on Bug 929865, we just process the .java.in files into the correct directories in m/a/b/Makefile.in. That is, let's write $OBJDIR/generated/org/mozilla/gecko and $OBJDIR/generated/org/mozilla/fennec_$USER, etc. I'll look into this solution.

There is now a |mach build-backend| in inbound. Conceptually, I think it makes sense to roll the "projectify" code path into this command so we don't have multiple commands that output files for managing builds. Whatever you do, I will happily r+ patches that add notices to build output that tells builders of mobile/android that they should generate IDE project files: we should nudge developers into having the best development experience by default. I'd even go so far as to suggest we generate project files automatically as part of config.status [unless this is a build in automation].

Hmm. After actually looking at the patch, perhaps we should just make this |mach create-fennec-project| (or similar - naming is hard) and set a filter on the command so that it only shows up if MOZ_BUILD_APP == 'mobile/android'.

Comment on attachment 825966 [details] [diff] [review] Implement |mach projectify-{create,update}|. This depends on the two patches from Bug 933300. This breaks projectify into |projectify-create| (which is significantly simpler than it used to be) and |projectify-update| (which is both simpler and more powerful). You'll need to run update before create because creating requires preprocessed things to be in place. Screencast and more documentation to follow, and this needs more polish.

Comment on attachment 825966 [details] [diff] [review] Implement |mach projectify-{create,update}|. Review of attachment 825966 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/projectify/projectify.py @@ +296,5 @@ > + if f not in sources: > + excluding_set.add(f) > + > + excluding_list = [ '|' + os.path.join('org/mozilla/gecko', f[len(base)+1:]) > + for f in sorted(excluding_set) if f.startswith(base) ] Why do we need this `f.startswith(base)` condition when self._find() looks for java files only in base? @@ +297,5 @@ > + excluding_set.add(f) > + > + excluding_list = [ '|' + os.path.join('org/mozilla/gecko', f[len(base)+1:]) > + for f in sorted(excluding_set) if f.startswith(base) ] > + return ''.join(excluding_list) We'll probably want the same exclusion list in IntelliJ, so I'd move this method to ProjectCreator and have it just return an array of exclusions, and each IDE creator can format the result.

This version handles |mach projectify-create| from a cold start (previous version required projectify-update first). Lots of other improvements too.

Comment on attachment 824289 [details] [diff] [review] Minor fixes (FOLD ME) Review of attachment 824289 [details] [diff] [review]: ----------------------------------------------------------------- We'll get back to this after I PTO.

Comment on attachment 8347784 [details] [diff] [review] Implement |mach projectify-{create,update}|. Unfortunately, I won't have time to push this forward for weeks/months. I'm hoping that gps will understand and agree to commit this version so we can get some testing and visibility before the end of Q4, and then others can help out. Known issues: * IntelliJ support is (presumably) broken. I'd rather commit a little broken code and open a follow-up to fix rather than possibly lose the code. * The class comment needs to be updated. This needs a documentation pass for dev-doc-needed and blog posts, at which time we can update the comment. * There's a good deal of hard-coding. I have substantial WIP for Bug 929298 but I don't expect to be able to push that forward until well into Q1, so we can't block on getting more out of moz.build. * This needs another refactor to generalize to other projects (GeckoView library and app, Robocop, etc). But that probably needs to block on Bug 929298. * There is a hard-coded path in cram tests. These are just for my sanity; they're not running anywhere (yet?) but I want to keep them for at least a while.

Comment on attachment 8349649 [details] [diff] [review] Implement |mach projectify-{create,update}|. Updated to avoid error with resource directories, and to have defaults easier for early testing.

Comment on attachment 8347784 [details] [diff] [review] Implement |mach projectify-{create,update}|. Review of attachment 8347784 [details] [diff] [review]: ----------------------------------------------------------------- What a patch! I have little clue what all the new project files do. But, I assume they are all necessary. I'm intrigued by the addition of the Ant files. While Ant only appears to have partial support for building, I'm wondering if you have larger aspirations here. If you wanted to replace make with Ant for building Fennec, I would be supportive of this effort :) It's buried in one of the inline comments, but I'd like you to consider reimplementing the project file generation as a build backend and not as a piece of standalone code. That would make this code clearly part of the build module and I feel less prone to breakage. The big barrier to r+ is the general hackiness of this code. I /really/ want to see this patch landed, as it will make developers' lives so much better (I think). That's worthy of sacrificing some quality, IMO. And, it's not like the Fennec build code isn't already full of hack :) I just think we can do a little better to avoid the gratuitous bits. At the very least, we can have bugs/comments annotating the parts that are known hacky. ::: mobile/android/mach_commands.py @@ +20,5 @@ > + > +@CommandProvider > +class MachCommands(MachCommandBase): > + @Command('projectify-create', > + description='Create IDE projects for Fennec.', category='devenv') How will we handle IDE project generation for Visual Studio for Gecko builds? Perhaps we should call this 'android-ide' or 'fennec-ide' or similar. @@ +24,5 @@ > + description='Create IDE projects for Fennec.', category='devenv') > + @CommandArgument('srcdir', default=os.path.join('mobile', 'android', 'base'), > + help='Project source directory.') > + @CommandArgument('--ide', '-i', > + help='IDE name.') Should we specify a "choices" argument to limit to known project types? @@ +54,5 @@ > + print('IDE name must be provided. Valid IDEs: ' + ', '.join(creators.keys())) > + return 1 > + if not workspace: > + print('IDE workspace directory must be provided.') > + return 1 Typically required arguments aren't - or -- prefixed. self._mach_context.state_dir contains the "mozbuild" state directory (~/.mozbuild by default). Perhaps that would make a reasonable default location? Could we also stuff it into the objdir? Does that make sense, or will people modify their projects frequently? @@ +63,5 @@ > + assets_src_dir = None > + lib_src_dir = None > + > + # XXX Get this from moz.build? Not clear how this might work. > + if srcdir == 'mobile/android/base': What about backslashes on Windows? import mozpack.mozpath as mozpath mozpath.normpath() (mozpath uses forward slashes for everything and contains some nice APIs in addition to the os.path ones) @@ +87,5 @@ > + @CommandArgument('-s', '--silent', action='store_true', > + help='Log absolutely no output.') > + @CommandArgument('-v', '--verbose', action='store_true', > + help='Log output from sub-commands.') > + def update(self, srcdir, silent=False, verbose=False): It would be /really/ cool if some kind of hook in the IDE could do this automagically. For example, cmake-derived Visual Studio projects will regenerate automatically if any of the build config files have changed. Not sure if you can do this in Eclipse. Presumably you can. Followup fodder. ::: mobile/android/projectify/projectify.py @@ +30,5 @@ > + def __init__(self, topsrcdir, settings, log_manager, > + srcdir, > + topobjdir=None): > + MozbuildObject.__init__(self, topsrcdir, settings, log_manager, > + topobjdir=topobjdir) Keep reminding me to fix MozbuildObject.__init__ to use super() and to make overriding the constructor easier. I had a patch somewhere that snuck an _init() into MozbuildObject that would get called as part of MozbuildObject.__init__. We should just use super() and named arguments everywhere. @@ +39,5 @@ > + # Cannot initialize these things in the constructor, since the > + # files they refer to might not exist at construction time. > + # That is, create calls update which makes sure the tree is > + # configured; before this happens, we can't extract data from > + # moz.build files. Hmmm. It sounds like you should use the conditional mach commands foo to make the mach commands conditional on having a config.status in the objdir or something. @@ +43,5 @@ > + # moz.build files. > + self._mozbuild_sandbox = None > + self.log(logging.INFO, 'projectify', > + { 'topsrcdir': self.topsrcdir, > + 'topobjdir': self.topobjdir, }, Nit: cuddle braces. @@ +54,5 @@ > + def mozbuild_sandbox(self): > + if self._mozbuild_sandbox is None: > + path = os.path.join(self.srcdir, 'moz.build') > + self._mozbuild_sandbox = MozbuildSandbox(self.config_environment, path) > + self._mozbuild_sandbox.exec_file(path, True) From a purity perspective, I'd much prefer you consume the output of emitter.py or at least BuildReader.walk_topsrcdir() or BuildReader.read_topsrcdir() because these are the "official" APIs for reading build config. In fact, when I think of IDE project generation, I think of a feature that builds on top of config.status and just implements an alternate build "backend" (as opposed to RecursiveMakeBackend). See python/mozbuild/mozbuild/config_status.py. @@ +117,5 @@ > + # Cannot initialize these things in the constructor, since the > + # files they refer to might not exist at construction time. > + # That is, create calls update which makes sure the tree is > + # configured; before this happens, we can't extract data from > + # moz.build files. It's best to avoid this situation all-together. @@ +199,5 @@ > + > + Directories needed to write output file will be created. > + ''' > + # Make sure we can actually write to output directory. > + ensureParentDir(output_filename) Why? I thought FileAvoidWrite created parent directories automagically. @@ +219,5 @@ > + 'Preprocessing {count} files.') > + for src, dst in files.items(): > + self._preprocess(src, dst) > + > + def _find(self, src, base, *names): Can you rewrite this to use mozpack.files.FileFinder? If not, why? @@ +251,5 @@ > + rel = f[l:] > + fns[f] = os.path.join(dst, rel) > + return fns > + > + def _update_or_create_symlink(self, src, dst): Use mozpack.files.AbsoluteSymlinkFile().copy() everywhere this function is used. @@ +299,5 @@ > + count += 1 > + if (count % 100 == 0): > + self.log(logging.INFO, 'projectify', > + {'count': count, 'total': total}, > + 'Created {count} of {total} symlinks.') You may want to construct a mozpack.copier.FileCopier and use it instead. All this talk about symlinks. Does Eclipse project generation not work on Windows? Are we missing a raise on Windows? @@ +380,5 @@ > + elif self.objdir in res_dir: > + package_name = 'org.mozilla.fennec.generatedresources' > + project_name = '%sGeneratedResources' % self.project_name > + else: > + raise Exception('Unrecognized Android resource directory "%s"' % res_dir) Boo. Perhaps we should add a variable to the moz.build sandbox to define these? @@ +407,5 @@ > + > + def _src_excluding(self): > + '''Return a list of source exclusions formatted for Eclipse .classpath files.''' > + excluding_list = [ '|' + os.path.join('org', 'mozilla', 'gecko', f[len(self.srcdir)+1:]) > + for f in sorted(self.excluded_source_files) ] Nit: cuddle @@ +493,5 @@ > + # If the tree has not been configured yet, we need to > + # configure. > + if not self._exists(self.topobjdir, 'Makefile'): > + if 0 != build.configure(): > + return 1 Eek. We really need a better API for ensuring build state. I hate seeing this logic baked into things outside of python/mozbuild. @@ +503,5 @@ > + # If the tree has not yet had a backend configured, we need to > + # do that too. This isn't a great test but it will do for > + # now. The reference to self.substs only works after the > + # above configure because the config environment is read on > + # demand. This is very fragile. Again, let's nip this in the bud and avoid it. @@ +517,5 @@ > + # trouble, let's always build. > + if not self._exists(config, 'buildid'): > + if 0 != self._run_make(directory=config, > + silent=not verbose, > + log=True, print_directory=True): If you land this, kittens will die. Let's add a buildid getter somewhere, possibly in MozbuildObject. @@ +525,5 @@ > + if not self._exists(self.bindir, 'chrome', 'en-US', 'locale', 'branding', 'brand.dtd'): > + if 0 != self._run_make(directory=branding, > + target='AB_CD=en-US', > + silent=not verbose, > + log=True, print_directory=True): Oy. @@ +544,5 @@ > + > + return self._run_make(directory=base, > + target=list(sorted(generated_sources)), > + silent=not verbose, > + log=True, print_directory=True) I'm getting the feeling that all of this manual makery is rather hacky and fragile. I might regret saying this, but I almost prefer we just add a special target to mobile/android/Makefile.in that builds everything needed to support IDE project generation. This way, it's obviously part of the build config and build maintainers know about it. As things are implemented in this patch, you have code sitting off in mobile/android that isn't clearly part of the build config and I worry that lack of clarity makes it more fragile than necessary.

(In reply to Gregory Szorc [:gps] from comment #25) > Comment on attachment 8347784 [details] [diff] [review] > Implement |mach projectify-{create,update}|. > > Review of attachment 8347784 [details] [diff] [review]: > ----------------------------------------------------------------- > > What a patch! > > I have little clue what all the new project files do. But, I assume they are > all necessary. We think they are. They're basically copied from a fresh Eclipse or IntelliJ build, and then manually modified as necessary. > I'm intrigued by the addition of the Ant files. While Ant only appears to > have partial support for building, I'm wondering if you have larger > aspirations here. If you wanted to replace make with Ant for building > Fennec, I would be supportive of this effort :) I know nothing about Ant. I'd rather not introduce it for anything, since I think there are better Android build systems around (Buck?); but we might get to that point. > It's buried in one of the inline comments, but I'd like you to consider > reimplementing the project file generation as a build backend and not as a > piece of standalone code. That would make this code clearly part of the > build module and I feel less prone to breakage. My problem is that I don't know what making a build backend really entails. I'm not necessarily against it, but I don't know the scope. I have essentially zero paid hours for this until end of January at least. > The big barrier to r+ is the general hackiness of this code. I /really/ want > to see this patch landed, as it will make developers' lives so much better > (I think). That's worthy of sacrificing some quality, IMO. And, it's not > like the Fennec build code isn't already full of hack :) I just think we can > do a little better to avoid the gratuitous bits. At the very least, we can > have bugs/comments annotating the parts that are known hacky. There's no two ways about it, this is hacky. I have a path in my head for making it less hacky -- finish moving Fennec to moz.build, basically -- and I'll explain some of the choices inline, but this is hacky. > ::: mobile/android/mach_commands.py > @@ +20,5 @@ > > + > > +@CommandProvider > > +class MachCommands(MachCommandBase): > > + @Command('projectify-create', > > + description='Create IDE projects for Fennec.', category='devenv') > > How will we handle IDE project generation for Visual Studio for Gecko > builds? Perhaps we should call this 'android-ide' or 'fennec-ide' or similar. zfg. > @@ +24,5 @@ > > + description='Create IDE projects for Fennec.', category='devenv') > > + @CommandArgument('srcdir', default=os.path.join('mobile', 'android', 'base'), > > + help='Project source directory.') > > + @CommandArgument('--ide', '-i', > > + help='IDE name.') > > Should we specify a "choices" argument to limit to known project types? Happy to. At one point the choices were dynamically extracted from the module, which isn't loaded at mach time. > @@ +54,5 @@ > > + print('IDE name must be provided. Valid IDEs: ' + ', '.join(creators.keys())) > > + return 1 > > + if not workspace: > > + print('IDE workspace directory must be provided.') > > + return 1 > > Typically required arguments aren't - or -- prefixed. Known, but positional args in mach were giving me headaches. (And there were other required args earlier.) > self._mach_context.state_dir contains the "mozbuild" state directory > (~/.mozbuild by default). Perhaps that would make a reasonable default > location? Could we also stuff it into the objdir? Does that make sense, or > will people modify their projects frequently? It can't go in the srcdir, since it references resources in objdir. It can't go in objdir, since we don't want to blow away the (possibly user-edited) projects when we clean, or upgrade, or whatever. I'm not against putting the projects in the "mozbuild" state directory, and letting users put the Eclipse workspace (which contains many projects) wherever they care. I'll try this on for size. > @@ +63,5 @@ > > + assets_src_dir = None > > + lib_src_dir = None > > + > > + # XXX Get this from moz.build? Not clear how this might work. > > + if srcdir == 'mobile/android/base': > > What about backslashes on Windows? > > import mozpack.mozpath as mozpath > mozpath.normpath() > > (mozpath uses forward slashes for everything and contains some nice APIs in > addition to the os.path ones) We can't build Fennec on Windows for reasons I don't understand. Until then, zfg. > @@ +87,5 @@ > > + @CommandArgument('-s', '--silent', action='store_true', > > + help='Log absolutely no output.') > > + @CommandArgument('-v', '--verbose', action='store_true', > > + help='Log output from sub-commands.') > > + def update(self, srcdir, silent=False, verbose=False): > > It would be /really/ cool if some kind of hook in the IDE could do this > automagically. For example, cmake-derived Visual Studio projects will > regenerate automatically if any of the build config files have changed. Not > sure if you can do this in Eclipse. Presumably you can. > > Followup fodder. I think IntelliJ is more clever than Eclipse. > ::: mobile/android/projectify/projectify.py > @@ +30,5 @@ > > + def __init__(self, topsrcdir, settings, log_manager, > > + srcdir, > > + topobjdir=None): > > + MozbuildObject.__init__(self, topsrcdir, settings, log_manager, > > + topobjdir=topobjdir) > > Keep reminding me to fix MozbuildObject.__init__ to use super() and to make > overriding the constructor easier. I had a patch somewhere that snuck an > _init() into MozbuildObject that would get called as part of > MozbuildObject.__init__. We should just use super() and named arguments > everywhere. Yeah, this wankery with cloning MozbuildObject and the constructor warnings in that source is weird. > @@ +39,5 @@ > > + # Cannot initialize these things in the constructor, since the > > + # files they refer to might not exist at construction time. > > + # That is, create calls update which makes sure the tree is > > + # configured; before this happens, we can't extract data from > > + # moz.build files. > > Hmmm. It sounds like you should use the conditional mach commands foo to > make the mach commands conditional on having a config.status in the objdir > or something. I'm not against it. This is going to apply lots of places, but: lots of the build system weirdness is to support building with an IDE when you have not built libxul.so locally. That is, with the current version of these scripts you can unpack a downloaded TBPL APK and just rebuild the Java layer. Hence the build system hackiness. We could lose this for v1. > @@ +43,5 @@ > > + # moz.build files. > > + self._mozbuild_sandbox = None > > + self.log(logging.INFO, 'projectify', > > + { 'topsrcdir': self.topsrcdir, > > + 'topobjdir': self.topobjdir, }, > > Nit: cuddle braces. > > @@ +54,5 @@ > > + def mozbuild_sandbox(self): > > + if self._mozbuild_sandbox is None: > > + path = os.path.join(self.srcdir, 'moz.build') > > + self._mozbuild_sandbox = MozbuildSandbox(self.config_environment, path) > > + self._mozbuild_sandbox.exec_file(path, True) > > From a purity perspective, I'd much prefer you consume the output of > emitter.py or at least BuildReader.walk_topsrcdir() or > BuildReader.read_topsrcdir() because these are the "official" APIs for > reading build config. In fact, when I think of IDE project generation, I > think of a feature that builds on top of config.status and just implements > an alternate build "backend" (as opposed to RecursiveMakeBackend). See > python/mozbuild/mozbuild/config_status.py. This is reasonable; it's just not how this code evolved. > @@ +117,5 @@ > > + # Cannot initialize these things in the constructor, since the > > + # files they refer to might not exist at construction time. > > + # That is, create calls update which makes sure the tree is > > + # configured; before this happens, we can't extract data from > > + # moz.build files. > > It's best to avoid this situation all-together. This is to support building a virgin tree with an unpacked APK. That is, this is all about lowering the barrier to entry for folks who develop IDE first (or only). Trying to explain to people who want to get developing quickly all the ins and outs of build backends and configure, etc, is not what I have been targeting. > @@ +199,5 @@ > > + > > + Directories needed to write output file will be created. > > + ''' > > + # Make sure we can actually write to output directory. > > + ensureParentDir(output_filename) > > Why? I thought FileAvoidWrite created parent directories automagically. Probably it does, but we do lots of symlinking (possibly before any such writing). I'll check to see if this is still necessary. > @@ +219,5 @@ > > + 'Preprocessing {count} files.') > > + for src, dst in files.items(): > > + self._preprocess(src, dst) > > + > > + def _find(self, src, base, *names): > > Can you rewrite this to use mozpack.files.FileFinder? If not, why? Possibly; I'll try. > @@ +299,5 @@ > > + count += 1 > > + if (count % 100 == 0): > > + self.log(logging.INFO, 'projectify', > > + {'count': count, 'total': total}, > > + 'Created {count} of {total} symlinks.') > > You may want to construct a mozpack.copier.FileCopier and use it instead. > > All this talk about symlinks. Does Eclipse project generation not work on > Windows? Are we missing a raise on Windows? Until we care about building Fennec on Windows, zfg. > @@ +380,5 @@ > > + elif self.objdir in res_dir: > > + package_name = 'org.mozilla.fennec.generatedresources' > > + project_name = '%sGeneratedResources' % self.project_name > > + else: > > + raise Exception('Unrecognized Android resource directory "%s"' % res_dir) > > Boo. Perhaps we should add a variable to the moz.build sandbox to define > these? Slated, but I can't block this on moz.build any longer. The slightly updated version moves this cruftiness into a function that does what moz.build might do in future. Don't forget, we don't have APK definition in moz.build at all yet :( I have significant WIP but no time now to complete. > @@ +407,5 @@ > > + > > + def _src_excluding(self): > > + '''Return a list of source exclusions formatted for Eclipse .classpath files.''' > > + excluding_list = [ '|' + os.path.join('org', 'mozilla', 'gecko', f[len(self.srcdir)+1:]) > > + for f in sorted(self.excluded_source_files) ] > > Nit: cuddle > > @@ +493,5 @@ > > + # If the tree has not been configured yet, we need to > > + # configure. > > + if not self._exists(self.topobjdir, 'Makefile'): > > + if 0 != build.configure(): > > + return 1 > > Eek. We really need a better API for ensuring build state. I hate seeing > this logic baked into things outside of python/mozbuild. Fair enough. This is what some part of mach does, though. And this is special, 'cuz I want to handle the virgin tree case. > @@ +503,5 @@ > > + # If the tree has not yet had a backend configured, we need to > > + # do that too. This isn't a great test but it will do for > > + # now. The reference to self.substs only works after the > > + # above configure because the config environment is read on > > + # demand. This is very fragile. > > Again, let's nip this in the bud and avoid it. > > @@ +517,5 @@ > > + # trouble, let's always build. > > + if not self._exists(config, 'buildid'): > > + if 0 != self._run_make(directory=config, > > + silent=not verbose, > > + log=True, print_directory=True): > > If you land this, kittens will die. Let's add a buildid getter somewhere, > possibly in MozbuildObject. Sure. > @@ +525,5 @@ > > + if not self._exists(self.bindir, 'chrome', 'en-US', 'locale', 'branding', 'brand.dtd'): > > + if 0 != self._run_make(directory=branding, > > + target='AB_CD=en-US', > > + silent=not verbose, > > + log=True, print_directory=True): > > Oy. Again, very special for virgin trees. Fixing this is so much more effort than it's worth. This is the cause of all sorts of pain. > @@ +544,5 @@ > > + > > + return self._run_make(directory=base, > > + target=list(sorted(generated_sources)), > > + silent=not verbose, > > + log=True, print_directory=True) > > I'm getting the feeling that all of this manual makery is rather hacky and > fragile. I might regret saying this, but I almost prefer we just add a > special target to mobile/android/Makefile.in that builds everything needed > to support IDE project generation. This way, it's obviously part of the > build config and build maintainers know about it. As things are implemented > in this patch, you have code sitting off in mobile/android that isn't > clearly part of the build config and I worry that lack of clarity makes it > more fragile than necessary. I mooted this more than once, and was advocating for it for a while in conversations with bnicholson. I got pretty close to just making |mach build gecko.ap_| work, which *is* the relevant build target. Then I pushed into virgin tree builds, and started to understand how bad the l10n story around strings.xml really is, and the single target got lost in the shuffle. In conclusion: I'm well aware of the gnarly nature of this code. We shouldn't block on build system and/or moz.build improvements forever. And I really don't have enough time to do this right in the near future. We either try to smooth the corners for immediate gains and a big "get back to this later" or we don't land it. I'll ping back after I investigate some of your points.

Comment on attachment 8349649 [details] [diff] [review] Implement |mach projectify-{create,update}|. Review of attachment 8349649 [details] [diff] [review]: ----------------------------------------------------------------- This worked fine for me.

Comment on attachment 8349649 [details] [diff] [review] Implement |mach projectify-{create,update}|. Review of attachment 8349649 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling review; I'm hoping to implement this in a different manner.

This is a simple solution to one class of minor problems I was seeing, namely that process_install_manifest.py removes empty directories that are managed by Eclipse.

This is less simple. What was happening is that the symlink dest/dir -> src/dir would get deleted and then immediately re-created. I'm not 100% certain, but I believe this causes Eclipse to churn, 'cuz it thinks the target files have been added and removed.

These aren't really reviewable, per se, so I'm breaking them out into a separate commit.

test_recursive_make.py depends on the test data added by this commit, so it's here rather than in the earlier commit. There are two parts to the Android backend. One, it processes template files (at |build-backend| time) to write the project files. At the same time, it writes a manifest to $OBJDIR/android_eclipse/$PROJECT_NAME.manifest saying what symlinks should be created. The recursive make backend adds the appropriate targets to update dependencies and install the written manifest. These targets are run every time Eclipse builds. If this approach meets your approval, I'll write an .rst explaining this in more detail.

I wondered about adding android-eclipse.mozbuild files, except some of the generated file lists would be shared between mobile/android/base/moz.build and such a mozbuild file, which would be easy to break.

This is trivial, but shouldn't land as is because it almost doubles the runtime of config_status.py. We could keep the relevant definitions in memory to re-use them, but I think there is state (obj.ack) attached, and it's not clear how to reset it. Feedback needed.

Comment on attachment 8371775 [details] [diff] [review] Part 1: Add Android Eclipse project templates to mozbuild. Review of attachment 8371775 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/templates/android_eclipse/build.xml @@ +7,5 @@ > + <property name="project_name" value="@IDE_PROJECT_NAME@"/> > + > + <exec executable="${topsrcdir}/mach" dir="${topsrcdir}" failonerror="true"> > + <arg value="build"/> > + <arg value="${objdir}/ANDROID_ECLIPSE_PROJECT_${project_name}"/> The one thing I would like to point out is that we're calling vanilla |mach build| (we need an executable to call, and including the make path is error prone) with strange paths, of the form "mobile/android/base/ANDROID_ECLIPSE_PROJECT_Fennec". This has the effect of making |mach build| run make -C mobile/android/base ANDROID_ECLIPSE_PROJECT_Fennec, since mach thinks we're trying to build a single file. Now that I think about it, this should run from ${topobjdir}, to make sure we get the correct mozconfig. Will bump at some point.

Comment on attachment 8371771 [details] [diff] [review] Pre: Don't write file in test_util.py. Review of attachment 8371771 [details] [diff] [review]: ----------------------------------------------------------------- This one's on me.

Comment on attachment 8371773 [details] [diff] [review] Part 0a: Add --no-remove-empty-directories to process_install_manifest.py. r=gps Review of attachment 8371773 [details] [diff] [review]: ----------------------------------------------------------------- This code is fine. But I can haz test, please?

Comment on attachment 8371774 [details] [diff] [review] Part 0b: Make FileCopier not delete symlinks to directories that will be re-created. r=gps Review of attachment 8371774 [details] [diff] [review]: ----------------------------------------------------------------- This solution is sadly too naive. First, mozpack.copier support for symlinks was only intended for files. If you want to support symlinked directories, I recommend adding a symlinked directory type. Otherwise, you're playing with fire relying on symlinks to directories actually working. IIRC there is no explicit test coverage for this working. You are just lucky. This gets an r- because it could break the build. Consider the scenario where we encounter a symlink to a directory under topsrcdir. With this code, that symlink is preserved and the build system may place files in the topsrcdir by way of following the symlink. Obviously not intended. I believe we have this code path in copier.py because I encountered it as part of developing support for symlinks: I managed to create files in topsrcdir, was like WTF, and added code to deal with it so it wouldn't happen again. I think I'd be OK with a mode to not remove symlinks to directories that would be recreated iff a) it is optional and not the default b) it has test coverage.

Comment on attachment 8371775 [details] [diff] [review] Part 1: Add Android Eclipse project templates to mozbuild. Review of attachment 8371775 [details] [diff] [review]: ----------------------------------------------------------------- I'll rubber stamp this. But only once you say it is done :)

Comment on attachment 8371776 [details] [diff] [review] Part 2: Add frontend and RecursiveMakeBackend for Android Eclipse projects. Review of attachment 8371776 [details] [diff] [review]: ----------------------------------------------------------------- Tests would be nice. Meh. ::: python/mozbuild/mozbuild/frontend/reader.py @@ +251,5 @@ > + if not name: > + raise Exception('Android Eclipse project cannot be registered without a name') > + > + if self['ANDROID_ECLIPSE_PROJECT_TARGETS'].has_key(name): > + raise Exception('Android Eclipse project has already been registered: %s' % name) if name in self['ANDROID...']: @@ +260,5 @@ > + > + def _add_android_eclipse_project(self, name, manifest): > + if not manifest: > + raise Exception('Android Eclipse project must specify a manifest') > + Nit: ws

(In reply to Gregory Szorc [:gps] from comment #44) > Comment on attachment 8371776 [details] [diff] [review] > Part 2: Add frontend and RecursiveMakeBackend for Android Eclipse projects. > > Review of attachment 8371776 [details] [diff] [review]: > ----------------------------------------------------------------- > > Tests would be nice. Meh. I'm not really sure what non-trivial tests of the frontend would look like. The frontend is tested by the backend tests, of course. There are tests for the recursive make backend in the next commit; they're not here since they depend on test data that's more naturally included later. I could include separate test data for recursive make, but 0fg.

I think the test in the next patch in the series is acceptable. If this becomes too important, POTB, or breaks frequently, we can always add test coverage. For now, improving the dev workflow trumps most.

(In reply to Gregory Szorc [:gps] from comment #41) > Comment on attachment 8371773 [details] [diff] [review] > Part 0a: Add --no-remove-empty-directories to process_install_manifest.py. > r=gps > > Review of attachment 8371773 [details] [diff] [review]: > ----------------------------------------------------------------- > > This code is fine. But I can haz test, please? Sure. I meant to say the two copier tests were experimental; didn't want to test code that is definitely not going to land. See your comments on the next patch :)

Comment on attachment 8371780 [details] [diff] [review] Part 3: Add AndroidEclipseBackend. Review of attachment 8371780 [details] [diff] [review]: ----------------------------------------------------------------- This is mostly a rubber stamp. I'm taking most of the specifics on faith. I didn't see anything too alarming. ::: python/mozbuild/mozbuild/backend/android_eclipse.py @@ +1,1 @@ > +from __future__ import unicode_literals Need moar MPL. @@ +51,5 @@ > + > + def consume_finished(self): > + # The common backend handles WebIDL and test files. We don't > + # handle these, so we don't call our superclass. > + pass Fun fact: If you have a docstring, you don't need a pass. But you don't have a docstring... @@ +53,5 @@ > + # The common backend handles WebIDL and test files. We don't > + # handle these, so we don't call our superclass. > + pass > + > + def _Element_for_classpathentry(self, cpe): Is there a reason you capitalized Element? @@ +92,5 @@ > + def _manifest_for_project(self, srcdir, project): > + manifest = InstallManifest() > + > + if project.manifest: > + manifest.add_symlink(os.path.join(srcdir, project.manifest), 'AndroidManifest.xml') Paths to manifests should be using mozpath. It only really matters on Windows. I know Android doesn't build there yet. But please don't create technical debt. @@ +95,5 @@ > + if project.manifest: > + manifest.add_symlink(os.path.join(srcdir, project.manifest), 'AndroidManifest.xml') > + > + if project.res: > + manifest.add_symlink(os.path.join(srcdir, project.res), 'res') Might need to rewrite this a bit to deal with directory symlinks. We'll see. @@ +103,5 @@ > + > + for cpe in project._classpathentries: > + manifest.add_symlink(os.path.join(srcdir, cpe.srcdir), cpe.dstdir) > + > + # JARs and native libraries go in the same place. This Nit: Don't need 2 spaces after periods. ::: python/mozbuild/mozbuild/test/backend/test_android_eclipse.py @@ +1,4 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + Nit: Trailing whitespace in this file.

Comment on attachment 8371781 [details] [diff] [review] Part 4: Add Android Eclipse projects to moz.build files. Review of attachment 8371781 [details] [diff] [review]: ----------------------------------------------------------------- rs=gps.

Comment on attachment 8371789 [details] [diff] [review] Part 5: Run AndroidEclipseBackend in config_status.py. Review of attachment 8371789 [details] [diff] [review]: ----------------------------------------------------------------- I'd only feed into the Android backend if the current app is mobile/android. You may also want to print the path to the Eclipse project file so this feature is initially advertised. We can remove that later. We definitely don't non-Fennec builds to incur a config.status cost. I'd prefer each config.status invocation were attached to at most one backend. But if we must split the output to multiple backends, we'll need a new API. That's probably more work than just invoking config.status multiple times or doing the double reader in-process, like you've done here.

(In reply to Gregory Szorc [:gps] from comment #42) > Comment on attachment 8371774 [details] [diff] [review] > Part 0b: Make FileCopier not delete symlinks to directories that will be > re-created. r=gps > > Review of attachment 8371774 [details] [diff] [review]: > ----------------------------------------------------------------- > > This solution is sadly too naive. > > First, mozpack.copier support for symlinks was only intended for files. If > you want to support symlinked directories, I recommend adding a symlinked > directory type. Otherwise, you're playing with fire relying on symlinks to > directories actually working. IIRC there is no explicit test coverage for > this working. You are just lucky. > > This gets an r- because it could break the build. Consider the scenario > where we encounter a symlink to a directory under topsrcdir. With this code, > that symlink is preserved and the build system may place files in the > topsrcdir by way of following the symlink. Obviously not intended. > > I believe we have this code path in copier.py because I encountered it as > part of developing support for symlinks: I managed to create files in > topsrcdir, was like WTF, and added code to deal with it so it wouldn't > happen again. Sorry for the screed, but: I agree with the motivation, but I think the implemented deterrent is wrong. Let me restrict to only a single copier. First, FileCopier starts by determining the set |required_dirs| for the set |dest_files|. It then has perfectly sensible code to ensure these are real directories, including sensible code for dealing with symlinks to directories. (Such symlinks are removed and real directories created. Given our goal of not writing to $srcdir, this seems prudent.) Second, FileCopier then walks the existing tree to determine what |existing_files| and |existing_dirs| are. Intermixed with this walk is code to remove symlinks to directories, no matter what. (With no accounting in |removed_*|, to boot.) The important fact: there is no way we will write *into* such a directory, since any directory we might write into is in |required_dirs|, and we already made sure each such directory is not a symlink. It's possible we would overwrite an existing symlink to a directory with a file of some type, but we already need to handle the case of overwriting a symlink to a non-directory, and I fail to see how this is different. Now, the deterrent might still have value in the case of multiple copiers. But the only case I can see where this might happen is when we have a copier write to destination output/, remove a symlinked output/linked_dir, and then a second copier write to destination output/linked_dir. If we remove the deterrent, then that second copier could write blindly to a symlinked directory, which might be in $srcdir. But that is explicitly part of the FileCopier contract: caller controls the destination tree. I argue that any copier that relies on a previous copier to prepare its directory structure in this way is broken (since copiers are supposed to deal only with files). It seems to me that the right thing to do is just add each symlink to a directory to |existing_files|. Then they will be removed as usual if |remove_unaccounted| is truthy (just like an unaccounted symlink to a file would be removed), and they will block a directory from getting deleted as empty (just like symlink to a file would). What am I missing? > I think I'd be OK with a mode to not remove symlinks to directories that > would be recreated iff a) it is optional and not the default b) it has test > coverage. I am happy to test the behaviour I describe above.

Trivial review is trivial.

This adds and uses a --backend parameter to config_status.py.

Comment on attachment 8375887 [details] [diff] [review] Part 6: Allow running AndroidEclipse backend in |mach build-backend|. r=gps Review of attachment 8375887 [details] [diff] [review]: ----------------------------------------------------------------- The last piece. This is so close I can taste it.

Comment on attachment 8375887 [details] [diff] [review] Part 6: Allow running AndroidEclipse backend in |mach build-backend|. r=gps Review of attachment 8375887 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/config_status.py @@ +122,5 @@ > log_level = logging.DEBUG if options.verbose else logging.INFO > log_manager.add_terminal_logging(level=log_level) > log_manager.enable_unstructured() > > + print('Reticulating splines for %s backend...' % options.backend, file=sys.stderr) Please don't change this line.

Sorry, complete list of changesets is really: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2ffdc5a9471 https://hg.mozilla.org/integration/mozilla-inbound/rev/f7452552f771 https://hg.mozilla.org/integration/mozilla-inbound/rev/09d16faf75e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/9075983ad5e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/e03462c12642 https://hg.mozilla.org/integration/mozilla-inbound/rev/e059cc39770e https://hg.mozilla.org/integration/mozilla-inbound/rev/0152d755e724 and follow-up: https://hg.mozilla.org/integration/mozilla-inbound/rev/33546ec4ba48

And second follow-up: http://hg.mozilla.org/integration/mozilla-inbound/rev/226462df194e

https://hg.mozilla.org/mozilla-central/rev/a2ffdc5a9471 https://hg.mozilla.org/mozilla-central/rev/f7452552f771 https://hg.mozilla.org/mozilla-central/rev/09d16faf75e1 https://hg.mozilla.org/mozilla-central/rev/9075983ad5e6 https://hg.mozilla.org/mozilla-central/rev/e03462c12642 https://hg.mozilla.org/mozilla-central/rev/e059cc39770e https://hg.mozilla.org/mozilla-central/rev/0152d755e724 https://hg.mozilla.org/mozilla-central/rev/33546ec4ba48 https://hg.mozilla.org/mozilla-central/rev/226462df194e