1507754 - platform.ini is incorrect - SourceRepository field points to a TB repo

Status: RESOLVED FIXED

(Thunderbird :: Build Config, defect)

Description

[Florent]

Attachment: Capture d’écran_2018-11-16_12-55-14.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

Install last Thunderbird 60.3.1 x86 (maybe same with x64 version)


Actual results:

In install path see file  platform.ini "Milestone" is 60.3.0 instead of 60.3.1


Expected results:

In platform.ini "Milestone" = 60.3.1

Comment 1

[Florent]

forget to say it's on Windows platform

Comment 2

[Wayne Mery (:wsmwk)]

related to Bug 1507618?

Comment 3

[Florent]

I think is related registry + platform.ini

Comment 4

[Rob Lemley [:rjl]]

Actually the Milestone piece of that file is correct.

platform.ini refers to the Gecko platform on version which Thunderbird is built. So the Milestone field is essentially a Firefox version, coming from <mozsrc>/config/milestone.txt. Most of the time the version of Thunderbird matches the Firefox/Gecko version, but in this case Thunderbird 60.3.1 was based on Firefox 60.3.0. (There was no FF 60.3.1).
The equivalent information referring to Thunderbird is found in application.ini.

However, the SourceRepository value in platform.ini is incorrect (again). It should point to a mozilla- mercurial repository, but it looks like it's pointing to comm- repositories again even on nightly.

[Build]
BuildID=20190219091222
Milestone=67.0a1
SourceRepository=https://hg.mozilla.org/comm-central
SourceStamp=dd4aa59c6a1271cbf6ca10813d73f62e7cb072d5

That hash is a mozilla-central hash, not a comm-central one.

Comment 5

[Rob Lemley [:rjl]]

Attachment: fix_platformini.patch

Patch that updates platform.ini according to Thunderbird's needs.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=35c9af68f9c6d66492f4a22e42419eab71156521

Comment 6

[Rob Lemley [:rjl]]

Comment on attachment 9060953 [details] [diff] [review]
fix_platformini.patch

patch does not work on CI

Comment 7

[Rob Lemley [:rjl]]

Attachment: Patch 1 - platformini.patch

The rather lengthy commit message should cover everything going on here.

This has potential to cause some disruption for packagers who modify
application.ini, platform.ini or source-repo.h files. I think I covered all
the possibilities though and left a way out if I missed something.

Comment 8

[Rob Lemley [:rjl]]

Attachment: Patch 2 - platformini_p2.patch

Generate an accurate and complete source-repo.h file.

Comment 9

[Rob Lemley [:rjl]]

Attachment: Patch 3 - platformini_p3.patch

And finally produce an accurate platform.ini file!

Comment 10

[Rob Lemley [:rjl]]

Attachment: Patch 1 - platformini.patch

Updated and rebased. I think it works in CI now. The try still failed
because I ran it before the rebase and there was unrelated bustage.

Comment 11

[Rob Lemley [:rjl]]

Attachment: Patch 2 - platformini_p2.patch

Just rebased.

Comment 12

[Rob Lemley [:rjl]]

Attachment: Patch 3 - platformini_p3.patch

Just rebased.

Comment 13

[Geoff Lankow (:darktrojan)]

Reading these patches, I wondered: would it not be tidier if we made our own platform.ini as a generated file, then overwrote the toolkit one with it? That would mean we had full use of Python instead of the twisted build system version, plus it would avoid using sed in the Makefile.

I think I'm happy with the way you've done it so far, but it seems like the hard way.

Comment 14

[Rob Lemley [:rjl]]

Attachment: Patch 4 - platformini_p4.patch

This is the last one.

Generate an accurate sourcestamp.txt file for source tarballs.
Uses a template instead of a bunch of awk and sed commands.

Comment 15

[Rob Lemley [:rjl]]

My latest try build with all applied patches looks good!
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c4f1d2a2e2acb8c2acaf6996fdc9947613b4a9c1

(In reply to Geoff Lankow (:darktrojan) from comment #13)

Reading these patches, I wondered: would it not be tidier if we made our own platform.ini as a generated file, then overwrote the toolkit one with it? That would mean we had full use of Python instead of the twisted build system version, plus it would avoid using sed in the Makefile.

Well it's not just platform.ini. That's what triggered this bug, but this rabbit hole is deep and confusing. source-repo.h gets used in about:buildconfig and there's a long standing bug to get the Thunderbird source repo listed there which I am finally able to resolve with these changes! (I think it's from 2008?)

Back to platform.ini, yes I don't like the sed rules either as they tend to break. Now that the rest of this is working the way I'd like I can take another look at that.

Comment 16

[Rob Lemley [:rjl]]

There is another approach that might make more sense long term. I'm not advocating for changing tactic now, because this works. This is sort of a vision for the future next time someone touches this code.

All the work done here to figure out changsets and repos with moz.configure will still apply, but rather than ultimately mangling platform.ini because it's "wrong" maybe the approach should be to go back to the beginning and let the MOZ_SOURCE_(REPO|CHANGESET) variables refer to Mozilla code, and then only have to create let's say MOZ_APP_SOURCE_(REPO|CHANGESET) variables that for Thunderbird at least would point to Comm repos. Then use those to generate application.ini/application.ini.h either by creating it ourselves or I could maybe convince the Mozilla build team to adopt some changes to the application.ini.in template that look for MOZ_APP_SOURCE_{REPO|CHANGESET) and then fallback to MOZ_SOURCE_(REPO|CHANGESET). That's sort of what I had written a while back, but then things got changed underneath me.

Comment 17

[Geoff Lankow (:darktrojan)]

Comment on attachment 9088445 [details] [diff] [review]
Patch 1 - platformini.patch

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

::: build/moz.configure/gecko_source.configure
@@ +105,5 @@
> +        # Mozilla code sets these, but they will be empty strinmgs if building
> +        # locally. With Taskcluster they should be set properly.
> +        # If these values are not set by moz.configure, when source-repo.h
> +        # gets generated during the build phase, it winds up pointing to
> +        # the Mozilla tree and just confuses everything. This way we are

There's a typo here, and I'm not entirely sure I understand the comment. What is set by Mozilla code? comm_repo and comm_rev?

::: mail/moz.configure
@@ +63,5 @@
>  
>      return namespace(version=rv[0],
>                       version_display=rv[1])
>  
> +

Empty line added why?

Comment 18

[Geoff Lankow (:darktrojan)]

Comment on attachment 9088446 [details] [diff] [review]
Patch 2 - platformini_p2.patch

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

::: build/source_repos.py
@@ +44,5 @@
> +def main(args):
> +    if args:
> +        func = globals().get(args[0])
> +        if func:
> +            return func(sys.stdout, *args[2:])

main should return a non-zero value if it gets to the end. Or raise, I guess, but you went to the trouble of calling sys.exit.

::: mail/app.mozbuild
@@ +7,5 @@
>  # m-c or c-c.
>  
> +GENERATED_FILES['source-repo.h'].script = 'comm/build/source_repos.py:source_repo_header'
> +
> +

Any reason for a second blank line here?

Comment 19

[Geoff Lankow (:darktrojan)]

Comment on attachment 9088447 [details] [diff] [review]
Patch 3 - platformini_p3.patch

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

::: mail/Makefile.in
@@ +18,5 @@
>  
>  # As a fallout from bug 1247162, the sourcestamp in application.ini and
>  # platform.ini are the same, which isn't a problem for Firefox, but
>  # it's not right for anything else. So we correct platform.ini here.
> +# The first exprexsion is a URL with / characters, so use _ for a delimeter

Try "expression" and "delimiter". :-)

Comment 20

[Rob Lemley [:rjl]]

Attachment: Patch 1 - platformini.patch

I updated the comments. Hopefully they make more sense now.

Comment 21

[Rob Lemley [:rjl]]

Attachment: Patch 2 - platformini_p2.patch

Added a return value. I borrowed that bit from variables.py, and that
doesn't have an explicit return either.

Comment 22

[Rob Lemley [:rjl]]

Attachment: Patch 3 - platformini_p3.patch

I guess my IDE doesn't bother to spell check Makefiles?

Comment 23

[Geoff Lankow (:darktrojan)]

Comment on attachment 9089529 [details] [diff] [review]
Patch 1 - platformini.patch

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

Just passing by…

::: build/moz.configure/gecko_source.configure
@@ +100,5 @@
> +        log.info("{}/rev/{}".format(comm_repo, comm_rev))
> +        return namespace(comm_repo=comm_repo, comm_rev=comm_rev)
> +
> +
> +# Read sourcestamp.txt and return the Thunderbkird source URL (with changeset).

I don't think your spell-checker works on this file either.

@@ +112,5 @@
> +    Determine the Thunderbird Mercurial repository and revision for this build
> +    when environment variables are not set.
> +
> +    When the environment variables are not set, eventually they get calculated
> +    by build/variables.py, but they will be set to the mozilla- repository\

Hmm.

Comment 24

[Rob Lemley [:rjl]]

Attachment: Patch 1 - platformini.patch

Changes comments only.

Comment 25

[Rob Lemley [:rjl]]

Attachment: Patch 2 - platformini_p2.patch

Changes to commit message only.

Comment 26

[Rob Lemley [:rjl]]

Revisions since the r+ have been comments and commit message fixes.
I expect the try build to turn out good.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2e8eafc1bc4e2d747bacc8d94b99a0c7f184e605

Comment 27

[Rob Lemley [:rjl]]

Attachment: Patch 3 - platformini_p3.patch

Not sure how that gecko_source.configure stuff got in there, it should have
been.

Comment 28

[Jorg K (CEST = GMT+2)]

0:06.69 Determining COMM source information from environment...
0:06.69 Determining COMM source information from Mercurial...
0:06.73 Traceback (most recent call last):
0:06.73 File "c:/mozilla-source/comm-central/configure.py", line 164, in <module>
0:06.73 sys.exit(main(sys.argv))
0:06.73 File "c:/mozilla-source/comm-central/configure.py", line 40, in main
0:06.73 sandbox.run(os.path.join(os.path.dirname(file), 'moz.configure'))
0:06.73 File "c:\mozilla-source\comm-central\python\mozbuild\mozbuild\configure_init_.py", line 498, in run
0:06.73 func(*args)
0:06.73 File "c:\mozilla-source\comm-central\python\mozbuild\mozbuild\configure_init_.py", line 542, in _value_for
0:06.73 return self.value_for_depends(obj)
0:06.73 File "c:\mozilla-source\comm-central\python\mozbuild\mozbuild\util.py", line 963, in method_call
0:06.73 cache[args] = self.func(instance, *args)
0:06.73 File "c:\mozilla-source\comm-central\python\mozbuild\mozbuild\configure_init.py", line 551, in value_for_depends
0:06.73 value = obj.result()
0:06.73 File "c:\mozilla-source\comm-central\python\mozbuild\mozbuild\util.py", line 963, in method_call
0:06.73 cache[args] = self.func(instance, *args)
0:06.73 File "c:\mozilla-source\comm-central\python\mozbuild\mozbuild\configure_init.py", line 157, in result
0:06.73 return self.func(*resolved_args)
0:06.73 File "c:\mozilla-source\comm-central\python\mozbuild\mozbuild\configure_init.py", line 1149, in wrapped
0:06.73 return new_func(*args, **kwargs)
0:06.73 File "c:/mozilla-source/comm-central/comm/build/moz.configure/gecko_source.configure", line 130, in comm_repo_heuristics
0:06.73 comm_rev = check_cmd_output(hg, '-R', paths.commtopsrcdir,
0:06.73 NameError: global name 'paths' is not defined
0:06.75 *** Fix above errors and then restart with
0:06.75 "./mach build"
0:06.76 mozmake: *** [client.mk:115: configure] Error 1

:-(

Comment 29

[Rob Lemley [:rjl]]

Attachment: Patch 1 - platformini.patch

This should fix that. Currently building locally.

Comment 30

[Jorg K (CEST = GMT+2)]

Care to upload part 2 which now doesn't apply any more?

Comment 31

[Rob Lemley [:rjl]]

Attachment: Patch 2 - platformini_p2.patch

As requested

Comment 32

[Rob Lemley [:rjl]]

Attachment: Patch 3 - platformini_p3.patch

as requested

Comment 33

[Rob Lemley [:rjl]]

Attachment: Patch 4 - platformini_p4.patch

as requested

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6468fdce4742
Check source repositories and changesets during configure. r=darktrojan
https://hg.mozilla.org/comm-central/rev/44f9a7cee435
Generate source-repo.h with complete source repository data. r=darktrojan
https://hg.mozilla.org/comm-central/rev/8802796b5903
Update platform.ini with correct Repository URL as well as changeset. r=darktrojan

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a034a173b805
Generate proper sourcestamp.txt for source tar files. r=darktrojan DONTBUILD

Comment 36

[Rob Lemley [:rjl]]

Attachment: bug_1507754_esr68_fix.patch

This patch is needed only on comm-esr68 after the others on this bug have
been applied. The build fails on esr68 without it.

Comment 37

[Jorg K (CEST = GMT+2)]

Are we porting this to TB 68? There aren't any approval flags set. In the last status meeting you said: "Uplift 1507754 and friends to 68.2" - which friends?

Comment 38

[Rob Lemley [:rjl]]

Comment on attachment 9097738 [details] [diff] [review]
bug_1507754_esr68_fix.patch

[Approval Request Comment] (applies to all patches on this bug)
Regression caused by (bug #): Many
User impact if declined: Minimal. Most users won't look at platform.ini
Testing completed (on c-c, etc.): These patches have been running on central for a few weeks now. There were some local build hiccups the first day, but otherwise have been good
Risk to taking this patch (and alternatives if risky): I consider this low risk. platform.ini and application.ini and the other things that derive from and display source code information within the app will be correct now!

I can land these if you want when the time comes. No merge conflicts, just additional changes because of differences on esr68.

Comment 39

[Jorg K (CEST = GMT+2)]

TB 68.2.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/0bd1a3c3bee51d0643e1f3e9511876d27df182c1
https://hg.mozilla.org/releases/comm-esr68/rev/001cc51ad0c711f8411dd86d9fe05cc593f261a8
https://hg.mozilla.org/releases/comm-esr68/rev/97517e9bd4f8e2fa4dfbe0bd70e613dbaaf30936
https://hg.mozilla.org/releases/comm-esr68/rev/b1785e6f307c6ab79f2ee515c151cf0f93146b2d
https://hg.mozilla.org/releases/comm-esr68/rev/b70cfc932d26e38d57711eabaded1c84a6a85939

Why did we never put this onto a beta? We could still ship it in TB 70 beta 4.