`rustc --version --verbose` will report the git commit hash it was built from. We want to record this at configure time and use it to generate source links in crash reports. Currently we get links like `obj-firefox/media/src/libcore/result.rs:687` mistakenly directed to hg.mozilla.org. This should instead be recognized as rust libstd code and directed to e.g. https://github.com/rust-lang/rust/blob/db2939409/src/libcore/result.rs#l687 for rust 1.8.0 stable.

I wasn't successful in cargo-culting a python port of rust.m4, so just do this in shell. Ted, calling AC_SUBST(RUSTC_HASH) puts the variable in config.status. Can we read that directly when generating the symbol table? Any other suggestions of how to get it into the scripts?

symbolstore.py already imports `buildconfig` and uses buildconfig.substs[...] for a few things: https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py We could do the same thing here. I'm not sure how exactly we'd identify Rust sources right now, if we don't have full paths to the source files we'd have to have some heuristics. Related, I should finish bug 1266368.

I finally spent a little time poking through the rustc source, and with a lot of help from eddyb on #rust-internals I filed an issue on the relative paths coming out of libcore: https://github.com/rust-lang/rust/issues/34179

This is useful for parsing semi-machinable command output into dicts. Review commit: https://reviewboard.mozilla.org/r/62172/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/62172/

This is hopefully more reliable than parsing just the summary line, and makes available other keys like the commit-hash which we'd like to pass to the debug symbol automation. Review commit: https://reviewboard.mozilla.org/r/62174/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/62174/

https://reviewboard.mozilla.org/r/62174/#review58942 ::: build/moz.configure/rust.configure:25 (Diff revision 1) > try: > - # TODO: We should run `rustc --version -v` and parse that output instead. > - version = Version(subprocess.check_output( > - [rustc, '--version'] > - ).splitlines()[0].split()[1]) > - return version > + out = subprocess.check_output( > + [rustc, '--version', '--verbose'] > + ).splitlines() > + summary = out[0] > + details = dict(map(str.strip, line.split(':', 2)) for line in out[1:]) You could rewrite that as: details = dict((s.strip() for s in line.split(':', 2)) for line in out[1:]) and not require map. ::: build/moz.configure/rust.configure:26 (Diff revision 1) > - # TODO: We should run `rustc --version -v` and parse that output instead. > - version = Version(subprocess.check_output( > - [rustc, '--version'] > - ).splitlines()[0].split()[1]) > - return version > + out = subprocess.check_output( > + [rustc, '--version', '--verbose'] > + ).splitlines() > + summary = out[0] > + details = dict(map(str.strip, line.split(':', 2)) for line in out[1:]) > + return (summary, details) That makes the function output not very nice, and, considering the contents of summary overlap with details, why return both? It would also be nicer if the "release" field was Version()ed so that dependees don't need to do it themselves.

(In reply to Mike Hommey [:glandium] from comment #6) > details = dict((s.strip() for s in line.split(':', 2)) for line in out[1:]) Ok, thanks! > That makes the function output not very nice, and, considering the contents > of summary overlap with details, why return both? So I can report the summary line? I guess it doesn't matter much, I've just found quoting actual output in the configure log helpful in past debugging. > It would also be nicer if the "release" field was Version()ed so that > dependees don't need to do it themselves. I started out writing a class to encapsulate this, but that wasn't allowed by the sandbox either. :/

(In reply to Ralph Giles (:rillian) needinfo me from comment #7) > (In reply to Mike Hommey [:glandium] from comment #6) > > > details = dict((s.strip() for s in line.split(':', 2)) for line in out[1:]) > > Ok, thanks! > > > That makes the function output not very nice, and, considering the contents > > of summary overlap with details, why return both? > > So I can report the summary line? I guess it doesn't matter much, I've just > found quoting actual output in the configure log helpful in past debugging. If you want it for debugging, you can just log it from the rustc_version function... with log.debug(). > > It would also be nicer if the "release" field was Version()ed so that > > dependees don't need to do it themselves. > > I started out writing a class to encapsulate this, but that wasn't allowed > by the sandbox either. :/ Now that you mention it, it would also be nicer as a namespace() than as a dict. something like: details = dict(s.strip() for s in line.split(':', 2)) for line in out[1:]) details['release'] = Version(details['release']) return namespace(**details) Or details = {} for line in out[1:]: k, v = line.split(':', 2) details[k.strip()] = Version(v.strip()) if k.strip() == 'release' else v.strip() return namespace(**details) Or return namespace(**{ k: Version(v) if k == 'release' else v for line in out[1:] for k, v in [(s.strip() for s in line.split(':', 2))] }) Or a variant along these lines

(In reply to Mike Hommey [:glandium] from comment #8) > If you want it for debugging, you can just log it from the rustc_version > function... with log.debug(). If you want something to visually print to the user, you can re-derive it from what you have in details. In fact, the summary is too verbose, most people will only care about the version itself, not the commit date, hash and the build date.

Comment on attachment 8767769 [details] Bug 1275424 - Parse `rustc --version --verbose` in moz.configure. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62174/diff/1-2/

Sorry, didn't see your follow-up comments. Will look at the namespace idea tomorrow, I wasn't familiar with them.

(In reply to Mike Hommey [:glandium] from comment #9) > (In reply to Mike Hommey [:glandium] from comment #8) > > If you want it for debugging, you can just log it from the rustc_version > > function... with log.debug(). > > If you want something to visually print to the user, you can re-derive it > from what you have in details. In fact, the summary is too verbose, most > people will only care about the version itself, not the commit date, hash > and the build date. Also note you weren't actually using the summary :)

Comment on attachment 8767769 [details] Bug 1275424 - Parse `rustc --version --verbose` in moz.configure. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62174/diff/2-3/

log.debug() the summary line. Nice how that only prints on exception! Only return the keys we care about in a namespace.

Comment on attachment 8767769 [details] Bug 1275424 - Parse `rustc --version --verbose` in moz.configure. https://reviewboard.mozilla.org/r/62174/#review58984 ::: build/moz.configure/rust.configure:24 (Diff revisions 1 - 3) > -def rustc_version(rustc): > +def rustc_info(rustc): > try: > out = subprocess.check_output( > [rustc, '--version', '--verbose'] > ).splitlines() > - summary = out[0] > + log.debug(out[0]) If you don't care about the output when no error occurs, you could replace both subprocess.check_output + log.debug with check_cmd_output() (same arguments and return value as subprocess.check_output), which will output the whole output for rustc --version --verbose if it returns a non-zero error code. ::: build/moz.configure/rust.configure:27 (Diff revisions 1 - 3) > + version=Version(info['release']), > + commit=info['commit-hash'], Technically, this might fail if release and/or commit-hash are not in the output for some reason, and there is nothing that would catch this error and make it nicer to the user. In fact, out[0] could fail as well.

https://reviewboard.mozilla.org/r/62174/#review59008 ::: build/moz.configure/rust.configure:25 (Diff revision 3) > try: > - # TODO: We should run `rustc --version -v` and parse that output instead. > - version = Version(subprocess.check_output( > - [rustc, '--version'] > - ).splitlines()[0].split()[1]) > - return version > + out = subprocess.check_output( > + [rustc, '--version', '--verbose'] > + ).splitlines() > + log.debug(out[0]) > + info = dict((s.strip() for s in line.split(':', 2)) for line in out[1:]) BTW, this should be line.split(':', 1)

Note: this is what the output looks for a distro rustc (Debian): $ rustc --version --verbose rustc 1.9.0 binary: rustc commit-hash: unknown commit-date: unknown host: x86_64-unknown-linux-gnu release: 1.9.0

(In reply to Mike Hommey [:glandium] from comment #15) > Comment on attachment 8767769 [details] > If you don't care about the output when no error occurs, you could replace > both subprocess.check_output + log.debug with check_cmd_output() (same > arguments and return value as subprocess.check_output), which will output > the whole output for rustc --version --verbose if it returns a non-zero > error code. Ok, that's cleaner. Thanks. > > + version=Version(info['release']), > > + commit=info['commit-hash'], > > Technically, this might fail if release and/or commit-hash are not in the > output for some reason, and there is nothing that would catch this error and > make it nicer to the user. In fact, out[0] could fail as well. Yes. I tried adding `except KeyError` and `except IndexError` clauses to the `try` but those aren't exposed to the sandbox. A generic `expect:` anything clause is too general and hides the stack trace if something unexpected happens. I can do is old-fashioned `if len(out) < 2` checks and `info.get()` dictionary lookups with explicit fallbacks? > BTW, this should be line.split(':', 1) Oops. > Note: this is what the output looks for a distro rustc (Debian): > > $ rustc --version --verbose > rustc 1.9.0 > [...] > commit-hash: unknown Charming. So we'll have to handle 'unknown' from the start, but at least I know what the fallback default should be.

Comment on attachment 8767769 [details] Bug 1275424 - Parse `rustc --version --verbose` in moz.configure. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62174/diff/3-4/

Comment on attachment 8767769 [details] Bug 1275424 - Parse `rustc --version --verbose` in moz.configure. https://reviewboard.mozilla.org/r/62174/#review59320 ::: build/moz.configure/rust.configure:20 (Diff revision 4) > - try: > - # TODO: We should run `rustc --version -v` and parse that output instead. > + if len(out) < 2: > + die('Short output getting rustc version.') Since you're not using out[0] directly, it's not going to be a problem. Interestingly, open-ended ranges work on empty lists even when their start is > 0, so out[1:] won't raise an exception. It will only return an empty list. In which case, you'll fall back on Version('0') and 'unknown', as if len(out) was > 1 but no info was there.

Comment on attachment 8767769 [details] Bug 1275424 - Parse `rustc --version --verbose` in moz.configure. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62174/diff/4-5/

Update to implement comment #20. I haven't gotten the VCSInfo updates working since; will land this patch in a separate bug since it does not harm.

Resummarizing. I fixed the issue I mentioned in comment 3 a while back, so we have almost enough information to do this properly. For any given source file in the debug info we need to be able to map it to a (repository, changeset, relative path within the repository). For source files in m-c this is easy because we have the info of the repo we're building from handy, and we have $topsrcdir. For code from the Rust standard library we can get the commit hash from rustc as this patch proposes, and we'll be getting absolute paths from the machine that originally compiled the libstd in use, but we'll need to figure out how to map those to the Rust repo and get their relative paths. If the paths on the Rust build machines are stable then we can just have a hardcoded list in symbolstore.py and use that. I don't really have a better idea offhand, short of adding the Rust $topsrcdir to the compiler's `--version -v` output.

I tested this on a local mac build and I got lines in XUL.sym like: FILE 16215 git:github.com/rust-lang/rust:libstd/collections/hash/map.rs:9f2abadca2d065bf81772cb84981d0a22d8e98b3 I pushed this to try so I can look at the generated symbols from other platforms to verify those. I looked up some arbitrary crash reports with `rust_panic` on the stack to verify that the source paths I hardcoded match what we're currently seeing: https://crash-stats.mozilla.com/report/index/0820107c-38a1-401a-8091-53e0b0170623 https://crash-stats.mozilla.com/report/index/35db4048-99ba-46d3-b9fb-a0f930170623 https://crash-stats.mozilla.com/report/index/f1fba4dc-abbb-4651-a9d7-6d7160170622

Comment on attachment 8880935 [details] bug 1275424 - hardcode Rust source paths in symbolstore.py. https://reviewboard.mozilla.org/r/152300/#review157398 This seems a bit fragile. But if it is the best we can do, it is the best we can do.

I think we'll be able to do better in the future. Rust recently implemented source path remapping for debug info: https://github.com/rust-lang/rust/pull/41508 If they enabled that for the official builds they could map the source paths to some fixed prefix.

I spot-checked the symbols from the try builds and they all have paths that map to the rust-lang repo, so I think this will be fine, if inelegant.

Pushed by tmielczarek@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3b06dbb28dd4 hardcode Rust source paths in symbolstore.py. r=gps