Require alphabetical order when assigning to order-independent moz.build variables

RESOLVED FIXED in mozilla24

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
mozilla24
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

roc suggested this on dev-platform. We assigning a block of values to an order-independent variable (such as XPIDL_SOURCES, EXPORTS, etc), we should require that the block be sorted in alphabetical order.

We should be able to do this by defining a custom list-derived class that overrides __add__, __iadd__, and extend.

We can only require alphabetical order inside appended lists - we can't guarantee sorted order across the entire variable. The reason is condition blocks. There is simply no way to enforce order is alphabetical short of requiring people to redefine multiple condition blocks. So, the following will error:

FOO += ['foo', 'bar']

whereas the following will work because there are two separate append calls:

FOO += ['foo']
FOO += ['bar']

The only way we get this better is with static analysis. And that's quite complicated. Let's get part of the way there, plant the seed that things should be sorted, then implement the remaining coverage later, if ever.
For my purposes I don't think the sortedness of the overall list matters at all. The reason I like file lists sorted is that it's easy to find an element in the list and it's easy and unambiguous where to add a new element to the list. In the latter case I must already know I want to add to a specific conditional block, and in the former case I probably do and anyway I can easily fall back to search. I can't imagine ever wanting to improve sortedness by moving conditional blocks.
Posted patch Sorted lists, v1 (obsolete) — Splinter Review
First stab at it. Works locally on Linux. Still needs tests for new code as well as a try run.

Does anyone mind if I land the moz.build resorting changes without explicit review? Seems trivial enough and if I missed up I think the build would go red pretty quickly...
How about this. I'll just get a quick review from mshal on just the resorting in existing files...

Will upload a new patch 2 once I basic tests written.
Assignee: nobody → gps
Attachment #744351 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #744354 - Flags: review?(mshal)
And here is the Python class and tests.
Attachment #744361 - Flags: review?(ted)
https://tbpl.mozilla.org/?tree=Try&rev=87c028d33241

I expect failures due to unsorted lists in the wild. Will follow up with a version that prints warnings so I know what it will take to get TBPL green.
(In reply to Gregory Szorc [:gps] from comment #3)
> Created attachment 744354 [details] [diff] [review]
> Part 1: Sort existing, v1
> 
> How about this. I'll just get a quick review from mshal on just the
> resorting in existing files...
> 
> Will upload a new patch 2 once I basic tests written.

The patch looks good, though the sorting used seems to differ from the 'sort' binary (at least on my machine :). Is that what we want? For example, I had been using vim markers to pipe the lists through sort to arrange them. However, when I do that on one of the one's you've modified (accessible/public/moz.build, XPIDL_SOURCES), some of the filenames move around. Is the sorting methodology locale specific? How should developers sort things properly, if not using sort(1) ?
I'm using sorted() on a list of unicode. I /thought/ the default implementation of sorted() used a locale-agnostic byte comparison. Which in our case should behave like US-ASCII since we don't have any "high byte" code points.

Contrast this with GNU coreutils `sort` which I believe will utilize the system's current locale. Although, the man page says nothing of it. So, I dunno.
(In reply to Michael Shal [:mshal] from comment #6)
> Is the sorting methodology locale specific? How should developers
> sort things properly, if not using sort(1) ?

I'm not gps, but... reading the "other" patch on this bug he uses pythons builtin sorted() function, which when not passed a cmp argument does: cmp=lambda x,y: cmp(x.lower(), y.lower())

so it uses the compare builtin to compare the case-insens strings. It does not do so based on system locale. (presumably sorts by ascii codepage, though I did not verify that, but all googling could give me when looking for stuff about system local and python cmp was ways to make python use system codepage)

The case insens is likely your largest confusion between builtin python sort and system sort binary.
Where do you see that the default behavior of sorted() applies .lower()? My observation is things are case sensitive (implying byte order):

sorted(['AB', 'Aa'])
['AB', 'Aa']

We should probably explicitly define a comparison function, just to be sure.
My question on locale came about from my searching, where I stumbled across: http://stackoverflow.com/questions/8776807/is-pythons-sort-function-the-same-as-linuxs-sort-with-lc-all-c

Apparently I don't have LC_ALL=C set, according to 'locale':
LC_ALL=

If I explicitly run vim as 'LC_ALL=C vi accessible/public/moz.build' then when I filter through sort it matches the sorting in gps' patch. So, I guess this is on my end? I don't think it's something I set when setting up the machine, so it is likely to trip up others as well.
Comment on attachment 744354 [details] [diff] [review]
Part 1: Sort existing, v1

Review of attachment 744354 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, though I think we could use some guidelines for proper locale settings to ensure the sort command sorts the same as what python is expecting for moz.build.
Attachment #744354 - Flags: review?(mshal) → review+
(In reply to Gregory Szorc [:gps] from comment #9)
> Where do you see that the default behavior of sorted() applies .lower()? My
> observation is things are case sensitive (implying byte order):

Maybe I'm misreading then: http://docs.python.org/2/library/functions.html#sorted
(In reply to Justin Wood (:Callek) from comment #12)
> (In reply to Gregory Szorc [:gps] from comment #9)
> > Where do you see that the default behavior of sorted() applies .lower()? My
> > observation is things are case sensitive (implying byte order):
> 
> Maybe I'm misreading then:
> http://docs.python.org/2/library/functions.html#sorted

You are; that's an example about how to use the 'cmp' argument.
That is some poorly-written documentation, it's pretty confusing.
I've just been trying these patches on Thunderbird, and found a potential issue, which could be an artifact of the c-c build system, but I thought it worth mentioning.

I'm trying this on a clobber build on Mac, what I'm finding is that when configure hits an error due to the wrong order, I need to clobber before I can build again. If I just try building again I get:

make -j12 -C /Users/moztest/comm/main/src/../tb
Makefile:19: config/autoconf.mk: No such file or directory
/Users/moztest/comm/main/src/config/config.mk:27: config/autoconf.mk: No such file or directory
make[1]: *** No rule to make target `/Users/moztest/comm/main/tb/config/autoconf.mk', needed by `backend.mk'.  Stop.
make: *** [build] Error 2

If this is just an issue with c-c, then I'm happy with that for now, as we're actively working towards cleaning up the c-c build system.
Blocks: 868393
No longer depends on: 868393
Landed part 1 to minimize chance of bit rot.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1242311151

Still need some more reordering to deal with other platforms.
Whiteboard: [leave open]
Comment on attachment 744361 [details] [diff] [review]
Part n: Require sorted lists, v1

Review of attachment 744361 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine aside from the hack in sandbox.py. I have an alternate proposal.

::: python/mozbuild/mozbuild/frontend/sandbox.py
@@ +138,5 @@
> +        if cls == StrictOrderingOnAppendList and not isinstance(value, cls) \
> +            and isinstance(value, list):
> +            value = cls(value)
> +
> +        if not isinstance(value, cls):

I wonder if it wouldn't be cleaner to extend the VARIABLES definition to be like:
(storage_type, input_type, default_value, docs)

So that you could write this like:
if not isinstance(value, default[1]):
   ...
value = default[1](value)

and then the VARIABLES entry could look like:
(StrictOrderingOnAppendList, list, [], "whatever")

::: python/mozbuild/mozbuild/util.py
@@ +302,4 @@
>  class MozbuildDeletionError(Exception):
>      pass
>  
> +

Extraneous newline?
Attachment #744361 - Flags: review?(ted)
I was testing some patches for c-c on Mac with this applied, and had to fix a few m-c things to get it to work, so here's the current reordering that we need for Mac.
Attachment #746823 - Flags: review?(gps)
Comment on attachment 746823 [details] [diff] [review]
Part 2: Sort existing Mac uses

Review of attachment 746823 [details] [diff] [review]:
-----------------------------------------------------------------

Oh lovely. Now only Windows to handle. And, I have a patch in my local queue that fixes Windows.
Attachment #746823 - Flags: review?(gps) → review+
Oh, please land immediately as part 2 so you don't get bitrotted. I'll rebase my patch as part 3 or something.
Comment on attachment 746823 [details] [diff] [review]
Part 2: Sort existing Mac uses

https://hg.mozilla.org/integration/mozilla-inbound/rev/db9527acf096
Attachment #746823 - Attachment description: Part 3: Sort existing Mac uses → Part 2: Sort existing Mac uses
Attachment #744361 - Attachment description: Part 2: Require sorted lists, v1 → Part n: Require sorted lists, v1
We now differentiate between the stored and incoming types on global
variables. If an incoming type is not the stored type but is an allowed
type, we construct the stored type from the incoming value.
We now differentiate between the stored and incoming types on global
variables. If an incoming type is not the stored type but is an allowed
type, we construct the stored type from the incoming value.
Attachment #750111 - Flags: review?(ted)
Attachment #750109 - Attachment is obsolete: true
I cannot verify because Try is down :/
Attachment #750163 - Flags: review?(ted)
Comment on attachment 750111 [details] [diff] [review]
Part 3: Allow limited type coercion in moz.build sandbox

Review of attachment 750111 [details] [diff] [review]:
-----------------------------------------------------------------

That looks a lot nicer, thanks!
Attachment #750111 - Flags: review?(ted) → review+
Comment on attachment 750162 [details] [diff] [review]
Part 4: More sorting of existing lists;

Review of attachment 750162 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/public/ia2/moz.build
@@ +34,5 @@
>      'IA2CommonTypes',
>  ]
>  
> +# EXPORTS expects a sorted list and adding the suffix may change sort order.
> +# So ensure we are always sorted.

I don't see how this can be true if the base names are all the same.
Attachment #750162 - Flags: review?(ted) → review+
Attachment #750163 - Flags: review?(ted) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0661e6dd5b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/94978dab7186

I'm pretty sure there are still some stragglers with the actual sorting. Since Try is now working again, I'll see about fixing them.
Try looks happy with very minor (not review worthy) changes, so I pushed everything.

https://hg.mozilla.org/integration/mozilla-inbound/rev/da470133a9ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fbd99874bad
Whiteboard: [leave open]
Target Milestone: --- → mozilla24
Blocks: 882160
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.