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

RESOLVED FIXED in Instantbird 47

Status

Instantbird
Other
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

trunk
Instantbird 47

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8684608 [details] [diff] [review]
build.mk tries to access the buildid before it is set
Attachment #8684608 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
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?
(Assignee)

Comment 4

3 years ago
(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.
(Assignee)

Comment 5

3 years ago
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.
Attachment #8684608 - Flags: review?(florian) → feedback?(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.
Attachment #8684608 - Flags: feedback?(florian) → feedback-
(Assignee)

Updated

3 years ago
Summary: build.mk tries to access the buildid before it is set → Avoid calls to printconfigsetting.py to get build variables before platform.ini exists
(Assignee)

Comment 7

3 years ago
Created attachment 8685407 [details] [diff] [review]
Stop build.mk from needing to access platform.ini
(Assignee)

Comment 8

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
Created attachment 8685409 [details] [diff] [review]
Avoid im/app/Makefile.in calling printconfigsetting.py before platform.ini exists
(Assignee)

Updated

3 years ago
Attachment #8685407 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Attachment #8684608 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
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.
Attachment #8685409 - Flags: feedback?(florian)
(Assignee)

Comment 11

3 years ago
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.
Attachment #8685409 - Flags: feedback?(florian)
(Assignee)

Comment 12

3 years ago
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.
Attachment #8685409 - Flags: feedback?(florian)
(Assignee)

Comment 13

3 years ago
Created attachment 8685502 [details] [diff] [review]
Stop build.mk from needing to access platform.ini
Attachment #8685502 - Flags: review?(florian)
(Assignee)

Comment 14

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8685407 - Attachment is obsolete: true
Attachment #8685407 - Flags: review?(florian)
Comment on attachment 8685502 [details] [diff] [review]
Stop build.mk from needing to access platform.ini

rs=me if this works.
Attachment #8685502 - Flags: review?(florian) → review+
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.
Attachment #8685409 - Flags: feedback?(florian)
(Assignee)

Comment 17

3 years ago
(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.
(Assignee)

Comment 18

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

Comment 19

3 years ago
Created attachment 8691059 [details] [diff] [review]
Remove unused GRE_MILESTONE, GRE_BUILDID compiler defines
Attachment #8691059 - Flags: review?(florian)
(Assignee)

Comment 20

3 years ago
(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.
(Assignee)

Comment 22

3 years ago
(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?).
(Assignee)

Comment 23

2 years ago
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
(Assignee)

Comment 24

2 years ago
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.
Attachment #8691059 - Flags: review?(florian) → review+
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 47
You need to log in before you can comment on or make changes to this bug.