1222740 - Avoid calls to printconfigsetting.py to get build variables before platform.ini exists

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect)

Description

[aleth [:aleth]]

Split off from TB Bug 1220501 - IOError: [Errno 2] No such file or directory: objdir/dist/bin/platform.ini

This happens because toolkit/mozapps/installer/package-name.mk (called from build.mk) runs before make-platformini.py has been called to set the buildid.

Comment 1

[aleth [:aleth]]

Attachment: build.mk tries to access the buildid before it is set

Comment 2

[aleth [:aleth]]

Turns out the build id is only required (in build.mk) to set other variables which are never used, either because they are commented out or because they are in #ifdefs which (as far as I can tell) are never true. That also explains why this bug didn't break anything in nightlies.

Since there's a lot of commented out code in this file, I commented out as opposed to deleting the unused lines.

Comment 3

[aleth [:aleth]]

Hmm, it appears I may have spoken too soon here.
Looking at a waterfall log of the distribute step, it seems LIST_PREVIOUS_MAR_CMD and UPLOAD_PATH are in fact set at that point (but where? buildbot?).

So maybe this patch won't work and the 'correct' solution would be to move all the stuff that requires the buildid under "distribution:" so it doesn't run during the build step?

Comment 4

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #0)
> This happens because toolkit/mozapps/installer/package-name.mk (called from
> build.mk) runs before make-platformini.py has been called to set the buildid.

For the record, make-platformini is called a couple of times. With one argument (--print-buildid) it prints the current time in the form of a build id (used to set env vars), with another argument (--buildid) it takes such a build id (supplied from an env var) as an argument and prints platform.ini to stdout (used to actually make platform.ini). So just because make-platformini.py has been called once doesn't mean platform.ini exists.

Comment 5

[aleth [:aleth]]

Comment on attachment 8684608 [details] [diff] [review]
build.mk tries to access the buildid before it is set

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

f? as I don't fully understand how this was all supposed to work.

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8684608 [details] [diff] [review]
build.mk tries to access the buildid before it is set

This patch just comments out all the code used to generate partial mar files.

LIST_PREVIOUS_MAR_CMD is set in the environment by buildbot.

If the problem is that $(shell echo $(BUILDID) is executed too soon, maybe we should just replace the := operators by = operators in the BUILD_* variable definitions.

Comment 7

[aleth [:aleth]]

Attachment: Stop build.mk from needing to access platform.ini

Comment 8

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> If the problem is that $(shell echo $(BUILDID) is executed too soon, maybe
> we should just replace the := operators by = operators in the BUILD_*
> variable definitions.

This *appears* to work. Whether it works in the buildbot setting too idk.

Comment 9

[aleth [:aleth]]

Attachment: Avoid im/app/Makefile.in calling printconfigsetting.py before platform.ini exists

Comment 10

[aleth [:aleth]]

Comment on attachment 8685409 [details] [diff] [review]
Avoid im/app/Makefile.in calling printconfigsetting.py before platform.ini exists

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

I'm not sure why the execution order changed in the build system (which is what must have caused these bugs). 

For some reason, the rules that call printconfigsetting from im/app/Makefile are executed multiple times during a build. This patch avoids the early calls, but not the ones at the end of the build (at the end of the build is when you'd expect im/app to be processed, moz.build tries to set it up that way). It doesn't seem like the "right" fix but I have no idea how to properly diagnose this.

Comment 11

[aleth [:aleth]]

Comment on attachment 8685409 [details] [diff] [review]
Avoid im/app/Makefile.in calling printconfigsetting.py before platform.ini exists

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

I'm not sure why the execution order changed in the build system (which is what must have caused these bugs). 

For some reason, the rules that call printconfigsetting from im/app/Makefile are executed multiple times during a build. This patch avoids the early calls, but not the ones at the end of the build (at the end of the build is when you'd expect im/app to be processed, moz.build tries to set it up that way). It doesn't seem like the "right" fix but I have no idea how to properly diagnose this.

Comment 12

[aleth [:aleth]]

Comment on attachment 8685409 [details] [diff] [review]
Avoid im/app/Makefile.in calling printconfigsetting.py before platform.ini exists

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

I'm not sure why the execution order changed in the build system (which is what must have caused these bugs). 

For some reason, the rules that call printconfigsetting from im/app/Makefile are executed multiple times during a build. This patch avoids the early calls, but not the ones at the end of the build (at the end of the build is when you'd expect im/app to be processed, moz.build tries to set it up that way). It doesn't seem like the "right" fix but I have no idea how to properly diagnose this.

Comment 13

[aleth [:aleth]]

Attachment: Stop build.mk from needing to access platform.ini

Comment 14

[aleth [:aleth]]

Comment on attachment 8685502 [details] [diff] [review]
Stop build.mk from needing to access platform.ini

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

This one makes more sense. Sorry for the misunderstanding.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8685502 [details] [diff] [review]
Stop build.mk from needing to access platform.ini

rs=me if this works.

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8685409 [details] [diff] [review]
Avoid im/app/Makefile.in calling printconfigsetting.py before platform.ini exists

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

Is this fixing a problem that Thunderbird also has? If so, should we also fix something in mail/app/Makefile.in? And what about browser/? Is this patch making our makefiles differ more?

::: im/app/Makefile.in
@@ -4,5 @@
>  
>  AB_CD = $(MOZ_UI_LOCALE)
>  
> -GRE_MILESTONE = $(shell $(PYTHON) $(MOZILLA_SRCDIR)/config/printconfigsetting.py $(LIBXUL_DIST)/bin/platform.ini Build Milestone)
> -GRE_BUILDID = $(shell $(PYTHON) $(MOZILLA_SRCDIR)/config/printconfigsetting.py $(LIBXUL_DIST)/bin/platform.ini Build BuildID)

I would prefer if this didn't move.

@@ +15,5 @@
>  ifdef SOURCE_REPO
>  DEFINES += -DMOZ_SOURCE_REPO="$(SOURCE_REPO)"
>  endif
>  
> +ifneq ("$(wildcard $(LIBXUL_DIST)/bin/platform.ini)","")

I don't think the quotes are needed.

@@ +27,1 @@
>  DEFINES += \

I would prefer the ifneq line to move right about the DEFINES += ... line.

Comment 17

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> Is this fixing a problem that Thunderbird also has? If so, should we also
> fix something in mail/app/Makefile.in? And what about browser/? Is this
> patch making our makefiles differ more?

The TB equivalent is in Bug 1220501. Browser doesn't have it, it's apparently another split build system issue.

Comment 18

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/7b4727d6db45679bbc26de4fb8a2d679b2e57394
 Bug 1222740 - Stop build.mk from needing to access platform.ini. r=florian

Comment 19

[aleth [:aleth]]

Attachment: Remove unused GRE_MILESTONE, GRE_BUILDID compiler defines

Comment 20

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #19)
> Created attachment 8691059 [details] [diff] [review]
> Remove unused GRE_MILESTONE, GRE_BUILDID compiler defines

This seems a better approach, and assuming I haven't overlooked anything, it also fixes the platform.ini errors.

Comment 21

[Florian Quèze [:florian]]

Is http://mxr.mozilla.org/comm-central/source/im/app/application.ini#9 processed correctly without these defines?

Comment 22

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #21)
> Is http://mxr.mozilla.org/comm-central/source/im/app/application.ini#9
> processed correctly without these defines?

Yes, the entries are set (there isn't a difference between c-c and m-c buildid, is there?).

Comment 23

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/ddba7f8e793c2458e83c34d70c01d35ee95f5189
Bug 1222740 - Port bug 1220501 (Remove unused compile defines from mail/app/Makefile.in) for IB. rs=bustage fix attempt

Comment 24

[aleth [:aleth]]

Comment on attachment 8691059 [details] [diff] [review]
Remove unused GRE_MILESTONE, GRE_BUILDID compiler defines

This landed with r=gps for TB ages ago, landing it for IB now in an attempt to help clarify the current bustage.