1461851 - Properly add the source URL in the profiler metadata

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P2)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

In bug 1451853 I tried to add source URL in the profiler metadata, but it's not working. I'm investigating.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Bug 1461851 - Properly add the source URL in the profiler metadata.

Review commit: https://reviewboard.mozilla.org/r/244210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244210/

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 3

[Markus Stange [:mstange]]

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 4

[Markus Stange [:mstange]]

export MOZILLA_OFFICIAL=1

in the mozconfig might do the trick.
https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/old-configure.in#4296

Comment 5

[Mike Hommey [:glandium]]

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.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244210/diff/1-2/

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

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 8

[Mike Hommey [:glandium]]

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.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244210/diff/2-3/

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

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 11

[Panos Astithas (he/him) [:past] (please ni?)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244210/diff/3-4/

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

This seems better:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13595021e96b5a5d35f4b3e33f8cf23cd615c45e

Comment 13

[Mike Hommey [:glandium]]

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.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

(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 15

[Panos Astithas (he/him) [:past] (please ni?)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244210/diff/4-5/

Comment 16

[Mike Hommey [:glandium]]

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.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244210/diff/5-6/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/602bdd9d5a96
Properly add the source URL in the profiler metadata. r=glandium

Comment 19

[Raul Gurzau (:RaulG)]

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)

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244210/diff/6-7/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/314e3ae1520f
Properly add the source URL in the profiler metadata. r=glandium

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Fixed.

Comment 23

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/314e3ae1520f