Closed Bug 1507754 Opened Last year Closed 3 months ago

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

Categories

(Thunderbird :: Build Config, defect)

defect
Not set

Tracking

(thunderbird_esr6870+ fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 70+ fixed
thunderbird71 --- fixed

People

(Reporter: florent.lagoda, Assigned: rjl)

References

Details

Attachments

(6 files, 14 obsolete files)

9.76 KB, image/png
Details
13.37 KB, patch
Details | Diff | Splinter Review
4.34 KB, patch
Details | Diff | Splinter Review
1.48 KB, patch
Details | Diff | Splinter Review
3.63 KB, patch
Details | Diff | Splinter Review
4.90 KB, patch
Details | Diff | Splinter Review
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
forget to say it's on Windows platform
related to Bug 1507618?
I think is related registry + platform.ini

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.

Assignee: nobody → rob
Component: Installer → Build Config
Summary: platform.ini is incorrect → platform.ini is incorrect - SourceRepository field points to a TB repo
Target Milestone: --- → Thunderbird 60.0
Version: 60 → 67
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch fix_platformini.patch (obsolete) — Splinter Review
Attachment #9060953 - Flags: review?(geoff)
Comment on attachment 9060953 [details] [diff] [review]
fix_platformini.patch

patch does not work on CI
Attachment #9060953 - Flags: review?(geoff)
Attached patch Patch 1 - platformini.patch (obsolete) — Splinter Review
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.
Attachment #9088330 - Flags: review?(geoff)
Attachment #9060953 - Attachment is obsolete: true
Attached patch Patch 2 - platformini_p2.patch (obsolete) — Splinter Review
Generate an accurate and complete source-repo.h file.
Attachment #9088331 - Flags: review?(geoff)
Attached patch Patch 3 - platformini_p3.patch (obsolete) — Splinter Review
And finally produce an accurate platform.ini file!
Attachment #9088332 - Flags: review?(geoff)
Attached patch Patch 1 - platformini.patch (obsolete) — Splinter Review
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.
Attachment #9088445 - Flags: review?(geoff)
Attachment #9088330 - Attachment is obsolete: true
Attachment #9088330 - Flags: review?(geoff)
Attached patch Patch 2 - platformini_p2.patch (obsolete) — Splinter Review
Just rebased.
Attachment #9088446 - Flags: review?(geoff)
Attachment #9088331 - Attachment is obsolete: true
Attachment #9088331 - Flags: review?(geoff)
Attached patch Patch 3 - platformini_p3.patch (obsolete) — Splinter Review
Just rebased.
Attachment #9088447 - Flags: review?(geoff)
Attachment #9088332 - Attachment is obsolete: true
Attachment #9088332 - Flags: review?(geoff)

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.

Attached patch Patch 4 - platformini_p4.patch (obsolete) — Splinter Review
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.
Attachment #9088815 - Flags: review?(geoff)

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.

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 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?
Attachment #9088445 - Flags: review?(geoff) → review+
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?
Attachment #9088446 - Flags: review?(geoff) → review+
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". :-)
Attachment #9088447 - Flags: review?(geoff) → review+
Attachment #9088815 - Flags: review?(geoff) → review+
Attached patch Patch 1 - platformini.patch (obsolete) — Splinter Review
I updated the comments. Hopefully they make more sense now.
Attachment #9088445 - Attachment is obsolete: true
Attached patch Patch 2 - platformini_p2.patch (obsolete) — Splinter Review
Added a return value. I borrowed that bit from variables.py, and that
doesn't have an explicit return either.
Attachment #9088446 - Attachment is obsolete: true
Attached patch Patch 3 - platformini_p3.patch (obsolete) — Splinter Review
I guess my IDE doesn't bother to spell check Makefiles?
Attachment #9088447 - Attachment is obsolete: true
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.
Attached patch Patch 1 - platformini.patch (obsolete) — Splinter Review
Changes comments only.
Attachment #9089529 - Attachment is obsolete: true
Attached patch Patch 2 - platformini_p2.patch (obsolete) — Splinter Review
Changes to commit message only.
Attachment #9089530 - Attachment is obsolete: true

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

Keywords: checkin-needed
Attached patch Patch 3 - platformini_p3.patch (obsolete) — Splinter Review
Not sure how that gecko_source.configure stuff got in there, it should have
been.
Attachment #9089532 - Attachment is obsolete: true

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

:-(

Keywords: checkin-needed
This should fix that. Currently building locally.
Attachment #9090081 - Attachment is obsolete: true

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

As requested
Attachment #9090082 - Attachment is obsolete: true
as requested
Attachment #9090153 - Attachment is obsolete: true
as requested
Attachment #9088815 - Attachment is obsolete: true

Pushed by mozilla@jorgk.com:
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

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 60.0 → Thunderbird 71.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a034a173b805
Generate proper sourcestamp.txt for source tar files. r=darktrojan DONTBUILD
Regressions: 1578920
This patch is needed only on comm-esr68 after the others on this bug have
been applied. The build fails on esr68 without it.

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 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.
Attachment #9097738 - Flags: approval-comm-esr68?
Attachment #9097738 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.