912914 - Preserve targets and dependencies order when creating Makefiles with makeutil.py

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Preserve targets and dependencies order when creating Makefiles with makeutil.py

Also allow to add random statements (like variable assignment)

Comment 2

[Mike Hommey [:glandium]]

Attachment: Preserve targets and dependencies order when creating Makefiles with makeutil.py

Also allow to add random statements (like variable assignment)

Now with stable order of the removal guards.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 800127 [details] [diff] [review]
Preserve targets and dependencies order when creating Makefiles with makeutil.py

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

r- on the grounds that this removes consistent ordering.

I'm pretty sure we have code using makeutil.py (or at least intending to use makeutil.py) that is relying on the consistent ordering of the generated make file. This patch breaks that promise!

I'm going to argue there are use cases for both preserving call-time ordering *and* for ensuring sorted output ordering. I think makeutil.py should support both. As a parameter to __init__ perhaps?

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 800127 [details] [diff] [review]
Preserve targets and dependencies order when creating Makefiles with makeutil.py

(In reply to Gregory Szorc [:gps] from comment #3)
> Comment on attachment 800127 [details] [diff] [review]
> Preserve targets and dependencies order when creating Makefiles with
> makeutil.py
> 
> Review of attachment 800127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- on the grounds that this removes consistent ordering.
> 
> I'm pretty sure we have code using makeutil.py (or at least intending to use
> makeutil.py) that is relying on the consistent ordering of the generated
> make file.

The patch trades one consistent ordering with another one. It's not like using a set() and having the result in a random order depending on what the set internal hash table looks like.

That being said, if something is relying on a particular order of dependencies in a Makefile, then we have a serious race condition issue waiting to break the tree.

Moreover, that code is currently only used to create .pp files, which are only used to rebuild stuff when things change. Order doesn't matter for these use cases.

> This patch breaks that promise!

There never was such promise.

Please reconsider.

Comment 5

[Gregory Szorc [:gps]]

I wanted to use makeutil.py to generate some FileAvoidWrite'ed files as part of backend generation. Once we hook that up, we'll be modifying some auto-generated make files as part of config.status because ordering may not be stable. I was hoping makeutil.py would perform all the sorts on write. But if we need to do that on the caller side, we can do it on the caller side.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #5)
> I wanted to use makeutil.py to generate some FileAvoidWrite'ed files as part
> of backend generation. Once we hook that up, we'll be modifying some
> auto-generated make files as part of config.status because ordering may not
> be stable.

Ordering *is* stable.

Comment 7

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb81ac52275

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cb81ac52275