Properly add the source URL in the profiler metadata

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: past, Assigned: past)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

In bug 1451853 I tried to add source URL in the profiler metadata, but it's not working. I'm investigating.
I discovered that it's not enough for MOZ_SOURCE_URL to be defined, as in a nightly I don't get it in the profiler metadata, while I do see it in AppConstants.SOURCE_REVISION_URL. Looking at how that is structured I came up with this approach, but I'm not certain it is the correct one. There is no other C/C++ file in the tree doing this, so I'm kind of guessing at this point.
I don't think that's going to work either. The @NAME@ syntax doesn't work for the C++ preprocessor, it only works for our JS/CSS/XML preprocessor.

I think this may work:

aWriter.StringProperty("sourceURL", MOZ_SOURCE_URL);

Is there a way to build locally in such a way that MOZ_SOURCE_URL is defined, so that you could test this?

Sorry for not catching this during review.
Comment on attachment 8976012 [details]
Bug 1461851 - Properly add the source URL in the profiler metadata.

https://reviewboard.mozilla.org/r/244210/#review250206

Unfortunately, this can't work. As mentioned, the C/C++ preprocessor doesn't take @FOO@. You'd need to use something like NS_STRINGIFY(FOO). But that still won't work be the // in the url is considered as starting a comment. So all you get from including source-repo.h is "https:". source-repo.h would need to be changed to define the values as strings. But that in turn, might break the other places already using it.

That being said, if you need the source repository at runtime, you might as well add it to XREAppData.

::: tools/profiler/core/platform.cpp:1554
(Diff revision 1)
>  {
>    MOZ_RELEASE_ASSERT(CorePS::Exists() && ActivePS::Exists(aLock));
>  
>    aWriter.IntProperty("version", 9);
>  
> -#if defined(MOZ_SOURCE_URL)
> +#ifndef MOZ_SOURCE_URL

Keep this.
Attachment #8976012 - Flags: review?(mh+mozilla) → review-
Is this what you had in mind?

I'm still having trouble creating a local build that displays the source URL even in about:buildconfig. Adding the following in .mozconfig doesn't seem to be enough:

export MOZ_SOURCE_URL=http://foo
export MOZ_SOURCE_CHANGESET=bar
export MOZILLA_OFFICIAL=1
export TEST_MOZBUILD=1
export MOZ_AUTOMATION=1

I've also tried setting these with mk_add_options instead of export to no avail.
Comment on attachment 8976012 [details]
Bug 1461851 - Properly add the source URL in the profiler metadata.

https://reviewboard.mozilla.org/r/244210/#review251604

You don't need to add a field for the source url, when we already have the source repository and the source stamp in there, and the url can easily be derived from those. However, we don't actually read platform.ini anymore. Plus, it's not the actual source of the AppInfo data. application.ini is. It is processed with build/appini_header.py to produce a header containing the XREAppData.
Attachment #8976012 - Flags: review?(mh+mozilla) → review-
Priority: -- → P2
Hmm, even though this works, asan builds aren't happy:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec34c57f24ad723fec9624af9846157b06fbbb24

I'll tweak it a bit.
Comment on attachment 8976012 [details]
Bug 1461851 - Properly add the source URL in the profiler metadata.

https://reviewboard.mozilla.org/r/244210/#review253406

::: build/appini_header.py:61
(Diff revision 4)
>                   NULL, // copyright
>                   %(flags)s,
>                   "%(Gecko:minversion)s",
>                   "%(Gecko:maxversion)s",
>                   "%(Crash Reporter:serverurl)s",
> -                 %(App:profile)s
> +                 "%(App:profile)s",

This can't be right, considering App:profile contains the double quotes already (or is NULL in some cases, which your change turns into the string literal "NULL")

::: build/appini_header.py:63
(Diff revision 4)
>                   "%(Gecko:minversion)s",
>                   "%(Gecko:maxversion)s",
>                   "%(Crash Reporter:serverurl)s",
> -                 %(App:profile)s
> +                 "%(App:profile)s",
> +                 NULL, // UAName
> +                 "%(App:sourcerepository)s/rev/%(App:sourcestamp)s"

It would be better to expand to NULL when either of those is not provided.
Attachment #8976012 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #13)
> This can't be right, considering App:profile contains the double quotes
> already (or is NULL in some cases, which your change turns into the string
> literal "NULL")

Whoops, I guess I got carried away.

> It would be better to expand to NULL when either of those is not provided.

OK, done.
Comment on attachment 8976012 [details]
Bug 1461851 - Properly add the source URL in the profiler metadata.

https://reviewboard.mozilla.org/r/244210/#review253920

::: build/appini_header.py:42
(Diff revisions 4 - 5)
>          sys.exit(1)
>  
>      if 'Crash Reporter:serverurl' not in appdata:
>          appdata['Crash Reporter:serverurl'] = ''
>  
> -    if 'App:sourcerepository' not in appdata:
> +    if 'App:sourcerepository' not in appdata or 'App:sourcestamp' not in appdata:

it would feel more obvious if you reversed the branches, and just called this 'App:sourcerepository' in appdata and 'App:sourcestamp' in appdata.
Attachment #8976012 - Flags: review?(mh+mozilla) → review+
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/602bdd9d5a96
Properly add the source URL in the profiler metadata. r=glandium
Backed out changeset 602bdd9d5a96 (bug 1461851) for linting failures on /checkouts/gecko/build/appini_header.py on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/f8369de48c34d6b6eb3d147785aed27474a24e0d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=602bdd9d5a96432300e7e8bd69bf608eb16ff952

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=180936999&repo=autoland&lineNumber=63

Log snippet: 

[taskcluster 2018-05-30 15:58:32.034Z] === Task Starting ===
[setup 2018-05-30T15:58:32.529Z] run-task started
[cache 2018-05-30T15:58:32.535Z] cache /builds/worker/checkouts exists; requirements: gid=1000 uid=1000 version=1
[volume 2018-05-30T15:58:32.535Z] changing ownership of volume /builds/worker/.cache to 1000:1000
[volume 2018-05-30T15:58:32.535Z] volume /builds/worker/checkouts is a cache
[setup 2018-05-30T15:58:32.536Z] running as worker:worker
[vcs 2018-05-30T15:58:32.536Z] executing ['hg', 'robustcheckout', '--sharebase', '/builds/worker/checkouts/hg-store', '--purge', '--upstream', 'https://hg.mozilla.org/mozilla-unified', '--revision', '602bdd9d5a96432300e7e8bd69bf608eb16ff952', 'https://hg.mozilla.org/integration/autoland', '/builds/worker/checkouts/gecko']
[vcs 2018-05-30T15:58:32.676Z] (using Mercurial 4.5.2)
[vcs 2018-05-30T15:58:32.676Z] ensuring https://hg.mozilla.org/integration/autoland@602bdd9d5a96432300e7e8bd69bf608eb16ff952 is available at /builds/worker/checkouts/gecko
[vcs 2018-05-30T15:58:32.678Z] (existing repository shared store: /builds/worker/checkouts/hg-store/8ba995b74e18334ab3707f27e9eb8f4e37ba3d29/.hg)
[vcs 2018-05-30T15:58:33.143Z] (pulling to obtain 602bdd9d5a96432300e7e8bd69bf608eb16ff952)
[vcs 2018-05-30T15:58:33.485Z] searching for changes
[vcs 2018-05-30T15:58:40.921Z] adding changesets
[vcs 2018-05-30T15:58:40.952Z] 
[vcs 2018-05-30T15:58:40.959Z] changesets [>                                                           ]  1/40
[vcs 2018-05-30T15:58:40.959Z]                                                                                 
[vcs 2018-05-30T15:58:40.959Z] adding manifests
[vcs 2018-05-30T15:58:41.154Z] adding file changes
[vcs 2018-05-30T15:58:42.001Z] added 40 changesets with 608 changes to 573 files (+1 heads)
[vcs 2018-05-30T15:58:42.469Z] new changesets 347ccb93cb08:602bdd9d5a96
[vcs 2018-05-30T15:58:42.470Z] (purging working directory)
[vcs 2018-05-30T15:58:51.657Z] 
[vcs 2018-05-30T15:58:52.211Z] updating [===>                                                        ]  61/792
[vcs 2018-05-30T15:58:52.247Z]                                                                                 
[vcs 2018-05-30T15:58:52.247Z] 731 files updated, 0 files merged, 61 files removed, 0 files unresolved
[vcs 2018-05-30T15:58:52.471Z] updated to 602bdd9d5a96432300e7e8bd69bf608eb16ff952
[vcs 2018-05-30T15:58:52.510Z] PERFHERDER_DATA: {"framework": {"name": "vcs"}, "suites": [{"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "pull", "shouldAlert": false, "subtests": [], "value": 9.193756818771362}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "purge", "shouldAlert": false, "subtests": [], "value": 6.956554889678955}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "update", "shouldAlert": false, "subtests": [], "value": 3.044105052947998}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "overall", "shouldAlert": false, "subtests": [], "value": 19.796462059020996}]}
[vcs 2018-05-30T15:58:52.961Z] TinderboxPrint:<a href=https://hg.mozilla.org/integration/autoland/rev/602bdd9d5a96432300e7e8bd69bf608eb16ff952 title='Built from autoland revision 602bdd9d5a96432300e7e8bd69bf608eb16ff952'>602bdd9d5a96432300e7e8bd69bf608eb16ff952</a>
[task 2018-05-30T15:58:52.961Z] executing ['bash', '-cx', 'cd /builds/worker/checkouts/gecko && ./mach lint -l flake8 -f treeherder']
[task 2018-05-30T15:58:52.965Z] + cd /builds/worker/checkouts/gecko
[task 2018-05-30T15:58:52.965Z] + ./mach lint -l flake8 -f treeherder
[task 2018-05-30T15:58:53.738Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2018-05-30T15:58:53.738Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2018-05-30T15:58:55.370Z] Installing setuptools, pip, wheel...done.
[task 2018-05-30T15:58:56.514Z] running build_ext
[task 2018-05-30T15:58:56.514Z] building 'psutil._psutil_linux' extension
[task 2018-05-30T15:58:56.514Z] creating build
[task 2018-05-30T15:58:56.514Z] creating build/temp.linux-x86_64-2.7
[task 2018-05-30T15:58:56.514Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-05-30T15:58:56.514Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-05-30T15:58:56.514Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-05-30T15:58:56.514Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-05-30T15:58:56.514Z] creating build/lib.linux-x86_64-2.7
[task 2018-05-30T15:58:56.514Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-05-30T15:58:56.514Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-05-30T15:58:56.514Z] building 'psutil._psutil_posix' extension
[task 2018-05-30T15:58:56.514Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-05-30T15:58:56.514Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-05-30T15:58:56.514Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-05-30T15:58:56.514Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-05-30T15:58:56.514Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-05-30T15:58:56.514Z] 
[task 2018-05-30T15:58:56.515Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-05-30T15:59:26.992Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/build/appini_header.py:43:35 | missing whitespace around operator (E225)
Flags: needinfo?(past)
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/314e3ae1520f
Properly add the source URL in the profiler metadata. r=glandium
Fixed.
Flags: needinfo?(past)
https://hg.mozilla.org/mozilla-central/rev/314e3ae1520f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.