+++ This bug was initially created as a clone of Bug #1431879 +++ It seems bug 1431879 was not enough. Somehow, the order in which conditions are output is not stable, and while the code says "for condition in sorted(conditions)", when you look at e.g. media/webrtc/trunk/moz.build, they're not actually sorted. Which led to this change when I ran GnMozbuildWriter: https://hg.mozilla.org/integration/autoland/diff/972057926234/media/webrtc/trunk/moz.build And when running it again with https://hg.mozilla.org/integration/autoland/rev/6e7c5695e34172b4975c4ad566f6cba309a1580c backed out, I also get something different from what was in the tree.

There are two issues here: We need to sort the conditions when generating `dirs` entries at the root of the project. This is a simple fix. Some of the code to factor variables doesn't handle cases where flags or includes are duplicated. In this case I'm not sure whether it's better to replicate pointless duplicated flags/includes in gn-configs in the generated moz.build files or to just ignore duplicates and continue assuming nothing in an upstream gn file is depending on flag sequences like "-no-something .... -something ... -no-something", where omitting the last flag would change the result.

Comment on attachment 8983257 [details] Bug 1466254 - Re-generate gn moz.build files. https://reviewboard.mozilla.org/r/249116/#review255312 ::: media/webrtc/trunk/webrtc/common_audio/common_audio_sse2_gn/moz.build:173 (Diff revision 1) > > if CONFIG["MOZ_DEBUG"] == "1" and CONFIG["OS_TARGET"] == "WINNT": > > DEFINES["_HAS_ITERATOR_DEBUGGING"] = "0" > > +if CONFIG["CPU_ARCH"] == "x86" and CONFIG["HOST_CPU_ARCH"] == "x86" and not CONFIG["MOZ_DEBUG"] and CONFIG["OS_TARGET"] == "FreeBSD": This is really horrible. For one, we should detect that there's the same thing with CONFIG['MOZ_DEBUG'] == 1, and remove CONFIG['MOZ_DEBUG'] from the condition. Secondly, the HOST_CPU_ARCH check here is pointless. If you're not touching HOST_*FLAGS, you shouldn't be testing anything about HOST_*.

Comment on attachment 8983255 [details] Bug 1466254 - Sort conditions when generating the moz.build file at the root of a gn project. https://reviewboard.mozilla.org/r/249112/#review255626 ::: media/webrtc/trunk/moz.build:87 (Diff revision 1) > "/media/webrtc/trunk/webrtc/voice_engine/voice_engine_gn", > "/media/webrtc/trunk/webrtc/webrtc_common_gn", > "/media/webrtc/trunk/webrtc/webrtc_gn" > ] > > -if CONFIG["OS_TARGET"] == "WINNT": > +if CONFIG["OS_TARGET"] == "DragonFly": Re-generating the moz.build files with this patch doesn't seem super useful since things just move around again in the later patch. Eg: This first condition ends up being if CONFIG["OS_TARGET"] == "Darwin" instead of DragonFly.

Comment on attachment 8983256 [details] Bug 1466254 - Don't assume flags/includes only appear once when factoring gn-generated moz.build conditions. https://reviewboard.mozilla.org/r/249114/#review255872 ::: python/mozbuild/mozbuild/gn_processor.py:363 (Diff revision 1) > # `reference`. > common_value = reference.get(k) > if common_value: > if isinstance(input_value, list): > - input_value = set(input_value) > - reference[k] = [i for i in common_value if i in input_value] > + reference[k] = [i for i in common_value > + if input_value.count(i) == common_value.count(i)] I think this may be doing the wrong thing if common_value has ['-msse2'] and input_value has ['-msse2', '-msse2]'. Wouldn't we want the intersection to be ['-msse2'] in this case instead of []? So we want the result to have the min(input_value.count(i), common_value.count(i)) instead of only applying it when they are equal. I tried it as a quick test, and it seemed to remove the ugly conditionals that glandium pointed out. I'm not sure if it has other issues though.

Comment on attachment 8983256 [details] Bug 1466254 - Don't assume flags/includes only appear once when factoring gn-generated moz.build conditions. https://reviewboard.mozilla.org/r/249114/#review255872 > I think this may be doing the wrong thing if common_value has ['-msse2'] and input_value has ['-msse2', '-msse2]'. Wouldn't we want the intersection to be ['-msse2'] in this case instead of []? So we want the result to have the min(input_value.count(i), common_value.count(i)) instead of only applying it when they are equal. > > I tried it as a quick test, and it seemed to remove the ugly conditionals that glandium pointed out. I'm not sure if it has other issues though. I don't think so. The premise of this patch (which I'm willing to be persuaded on) is that if one config has the flag once, and another has it twice, we shouldn't factor these into a common condition, because the difference between those two cases would be lost. I might be misunderstanding your proposal though, so feel free to upload your proposed patch. The "ugly conditionals" are from factoring the our conditions sub-optimally, and while I can think of ways to improve that, these are automatically generated files, and their introduction doesn't slow down reading the tree. The point of this bug is just to make things deterministic so anybody re-generating the files isn't distracted by diffs unrelated to their change. The aesthetics of the output seems less important at this point.

(In reply to Chris Manchester (:chmanchester) from comment #8) > Comment on attachment 8983256 [details] > Bug 1466254 - Don't assume flags/includes only appear once when factoring > gn-generated moz.build conditions. > > https://reviewboard.mozilla.org/r/249114/#review255872 > > > I think this may be doing the wrong thing if common_value has ['-msse2'] and input_value has ['-msse2', '-msse2]'. Wouldn't we want the intersection to be ['-msse2'] in this case instead of []? So we want the result to have the min(input_value.count(i), common_value.count(i)) instead of only applying it when they are equal. > > > > I tried it as a quick test, and it seemed to remove the ugly conditionals that glandium pointed out. I'm not sure if it has other issues though. > > I don't think so. The premise of this patch (which I'm willing to be > persuaded on) is that if one config has the flag once, and another has it > twice, we shouldn't factor these into a common condition, because the > difference between those two cases would be lost. I might be > misunderstanding your proposal though, so feel free to upload your proposed > patch. > > The "ugly conditionals" are from factoring the our conditions sub-optimally, > and while I can think of ways to improve that, these are automatically > generated files, and their introduction doesn't slow down reading the tree. > The point of this bug is just to make things deterministic so anybody > re-generating the files isn't distracted by diffs unrelated to their change. > The aesthetics of the output seems less important at this point. Ahh, that makes sense. I double-checked my attempt, and while it removes some of those conditionals, it doesn't address all of them. I'm also not sure if the result is correct or not. I'll attach the patch anyway in case you think it is worth pursuing.

Comment on attachment 8983256 [details] Bug 1466254 - Don't assume flags/includes only appear once when factoring gn-generated moz.build conditions. https://reviewboard.mozilla.org/r/249114/#review255932

Comment on attachment 8983257 [details] Bug 1466254 - Re-generate gn moz.build files. https://reviewboard.mozilla.org/r/249116/#review255934 These match what it produces locally for me. If you have ideas on how to improve the conditonals, maybe file a followup and include some details there?

While the condition repetitions (e.g. MOZ_DEBUG/!MOZ_DEBUG) can be considered esthetics, the HOST_* thing I mentioned in comment 5 is not. In fact, that pretty much breaks cross compilation (like, building x86 on x86-64 ; it's only fine on automation because we make it believe host is x86 too).

Comment on attachment 8983255 [details] Bug 1466254 - Sort conditions when generating the moz.build file at the root of a gn project. https://reviewboard.mozilla.org/r/249112/#review255626 > Re-generating the moz.build files with this patch doesn't seem super useful since things just move around again in the later patch. Eg: This first condition ends up being if CONFIG["OS_TARGET"] == "Darwin" instead of DragonFly. I didn't intend to do that here at all, I'll fix that before landing.

(In reply to Mike Hommey [:glandium] from comment #13) > While the condition repetitions (e.g. MOZ_DEBUG/!MOZ_DEBUG) can be > considered esthetics, the HOST_* thing I mentioned in comment 5 is not. In > fact, that pretty much breaks cross compilation (like, building x86 on > x86-64 ; it's only fine on automation because we make it believe host is x86 > too). I will fix this in a follow up.

Pushed by cmanchester@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d41073f73725 Sort conditions when generating the moz.build file at the root of a gn project. r=mshal https://hg.mozilla.org/integration/autoland/rev/f7fa20cd1fdc Don't assume flags/includes only appear once when factoring gn-generated moz.build conditions. r=mshal https://hg.mozilla.org/integration/autoland/rev/a9491cea8974 Re-generate gn moz.build files. r=mshal

https://hg.mozilla.org/mozilla-central/rev/d41073f73725 https://hg.mozilla.org/mozilla-central/rev/f7fa20cd1fdc https://hg.mozilla.org/mozilla-central/rev/a9491cea8974