Comment on attachment 8483423 [details] [diff] [review] Replace add_tier_dir with DIRS Review of attachment 8483423 [details] [diff] [review]: ----------------------------------------------------------------- So, something in this patch has broken c-c pseudowork. After applying bug 1062219, bug 1062221, and bug 1062235, I get this error: 0:37.52 make[4]: *** ../ldap: No such file or directory. Stop. 0:37.52 /src/trunk/comm-central/mozilla/config/recurse.mk:82: recipe for target '../ldap/export' failed The backend.mk for $MOZ_OBJDIR lists ../ldap where it should be listing ldap ::: python/mozbuild/mozbuild/frontend/reader.py @@ -981,3 @@ > for relpath, child_metadata in recurse_info.items(): > - if 'check_external' in child_metadata: > - relpath = '/' + relpath I'm guessing it has to be this code that is removed. Roughly speaking, (thinking back to when I understood the changes in the first place), we needed the tier dirs to treat /foo as relative to either topsrcdir or external_source_dir. The normalize_path below internally only checks if a file may exist in the external_source_dir as well for relative paths, so the conversion of these paths from absolute to relative again screws things up.

I think I'll leave the necessary emitter.py/reader.py changes to bug 1062235. Or to a separate patch in this bug. In any case, gps can review the current patch ignoring the c-c issue.

Up to now, DIRS and TEST_DIRS were dumb values. This change makes them a list of ContextDerivedValues, and handles the fact that some types of paths are relative to the current source directory and others to the topsrcdir. This also makes us one step closer to fixing bug 991983. Note we can kill MozbuildSandbox.normalize_path as a followup. The direct goal here is to unbust c-c from the main patch in this bug.

With: - A fix for tests. - Additional tests. - A fix for the android build.

Comment on attachment 8484093 [details] [diff] [review] Change how DIRS and TEST_DIRS are handled Review of attachment 8484093 [details] [diff] [review]: ----------------------------------------------------------------- You should look at the path resolution foo in bug 976733.

(In reply to Gregory Szorc [:gps] (away Sep 10 through 27) from comment #8) > Comment on attachment 8484093 [details] [diff] [review] > Change how DIRS and TEST_DIRS are handled > > Review of attachment 8484093 [details] [diff] [review]: > ----------------------------------------------------------------- > > You should look at the path resolution foo in bug 976733. I think my approach scales better to other variables.

Comment on attachment 8483423 [details] [diff] [review] Replace add_tier_dir with DIRS Review of attachment 8483423 [details] [diff] [review]: ----------------------------------------------------------------- r+ conditional on not making comm-central people angry when this lands.

Comment on attachment 8484043 [details] [diff] [review] Add a TypedList type and refactor mozbuild.util lists Review of attachment 8484043 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/util.py @@ +776,5 @@ > setattr(instance, name, self.func(instance)) > return getattr(instance, name) > + > + > +class TypedListMixin(object): Docstring, please.

Comment on attachment 8484093 [details] [diff] [review] Change how DIRS and TEST_DIRS are handled Review of attachment 8484093 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/moz.build @@ -21,5 @@ > 'extensions', > ] > > -if not CONFIG['MOZ_ANDROID_MLS_STUMBLER']: > - DIRS.remove('stumbler') Never underestimate the ability of people to discover and fire foot guns. ::: python/mozbuild/mozbuild/frontend/context.py @@ +229,5 @@ > + # external source dir, use that as base instead of topsrcdir. > + if self.context.config.external_source_dir: > + ret = mozpath.join(self.context.config.external_source_dir, > + self.value[1:]) > + if not ret or not os.path.exists(ret): Having a non-existing path to an external source dir fall back to topsrcdir feels a bit weird. Please document the reasoning in comments.

Regarding SourcePath, it looks like your approach will take us down a separate class for different path locations road. But the paths behave like strings, so it should be all good.

(In reply to Gregory Szorc [:gps] (away Sep 10 through 27) from comment #10) > Comment on attachment 8483423 [details] [diff] [review] > Replace add_tier_dir with DIRS > > Review of attachment 8483423 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ conditional on not making comm-central people angry when this lands. I believe the newer patches get it working again, but I didn't try the latest version. (In reply to Gregory Szorc [:gps] (away Sep 10 through 27) from comment #12) > Having a non-existing path to an external source dir fall back to topsrcdir > feels a bit weird. Please document the reasoning in comments. I slightly dislike doing external source dir then topsrcdir, but I keep hoping that cc-rework (and the ability to rip all of this stuff out) is possible to happen soon...

(In reply to Gregory Szorc [:gps] (away Sep 10 through 27) from comment #13) > Regarding SourcePath, it looks like your approach will take us down a > separate class for different path locations road. But the paths behave like > strings, so it should be all good. There would be at most three: one for paths that can only be in the source, one for paths that can only be in objdir, and one for both.

> Having a non-existing path to an external source dir fall back to topsrcdir > feels a bit weird. Please document the reasoning in comments. Note that nothing is changing here. It's already the case.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2548fb4c6a https://hg.mozilla.org/integration/mozilla-inbound/rev/23eb4e460b71 https://hg.mozilla.org/integration/mozilla-inbound/rev/601b5bcf7af2

https://hg.mozilla.org/mozilla-central/rev/0a2548fb4c6a https://hg.mozilla.org/mozilla-central/rev/23eb4e460b71 https://hg.mozilla.org/mozilla-central/rev/601b5bcf7af2