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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Sorted lists, v1

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...

Comment 3

[Gregory Szorc [:gps]]

Attachment: 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.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part n: Require sorted lists, v1

And here is the Python class and tests.

Comment 5

[Gregory Szorc [:gps]]

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.

Comment 6

[Michael Shal [:mshal]]

(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) ?

Comment 7

[Gregory Szorc [:gps]]

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.

Comment 8

[Justin Wood (:Callek)]

(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.

Comment 9

[Gregory Szorc [:gps]]

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.

Comment 10

[Michael Shal [:mshal]]

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 11

[Michael Shal [:mshal]]

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.

Comment 12

[Justin Wood (:Callek)]

(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

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 14

[(not currently active) Ted Mielczarek]

That is some poorly-written documentation, it's pretty confusing.

Comment 15

[Mark Banner (:standard8)]

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.

Comment 16

[Gregory Szorc [:gps]]

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.

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6d1242311151

Comment 18

[(not currently active) Ted Mielczarek]

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?

Comment 19

[Mark Banner (:standard8)]

Attachment: Part 2: Sort existing Mac uses

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.

Comment 20

[Gregory Szorc [: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.

Comment 21

[Gregory Szorc [:gps]]

Oh, please land immediately as part 2 so you don't get bitrotted. I'll rebase my patch as part 3 or something.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 746823 [details] [diff] [review]
Part 2: Sort existing Mac uses

https://hg.mozilla.org/integration/mozilla-inbound/rev/db9527acf096

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/db9527acf096

Comment 24

[Gregory Szorc [:gps]]

Attachment: Part 3: Allow limited type coercion in moz.build sandbox

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.

Comment 25

[Gregory Szorc [:gps]]

Attachment: Part 3: Allow limited type coercion in moz.build sandbox

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.

Comment 26

[Gregory Szorc [:gps]]

Attachment: Part 4: More sorting of existing lists;

Comment 27

[Gregory Szorc [:gps]]

Attachment: Part 5: Require sorted lists in moz.build files;

I cannot verify because Try is down :/

Comment 28

[(not currently active) Ted Mielczarek]

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!

Comment 29

[(not currently active) Ted Mielczarek]

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.

Comment 30

[Gregory Szorc [:gps]]

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.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Part 4 backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/de69327ab4e2

https://tbpl.mozilla.org/php/getParsedLog.php?id=23036506&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=23036518&tree=Mozilla-Inbound

Comment 32

[Gregory Szorc [:gps]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0661e6dd5b4

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da470133a9ba
https://hg.mozilla.org/mozilla-central/rev/8fbd99874bad