Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1921532 - Update symbol upload scripts [ci full] #6400

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Sep 27, 2024

Copied the latest script from moz-central:
https://searchfox.org/mozilla-central/rev/837f3a1ff2622c8303750c35d84bdc41a5cd079c/toolkit/crashreporter/tools/symbolstore.py

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@bendk bendk requested a review from a team September 27, 2024 19:29
@bendk bendk changed the title Bug 1921532 - Update symbol upload scripts Bug 1921532 - Update symbol upload scripts [ci full] Sep 27, 2024
@bendk
Copy link
Contributor Author

bendk commented Sep 27, 2024

I didn't update dump_syms because when I search tooltool this seems to be the latest version. Is there a newer version we can use?

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.72%. Comparing base (c965aab) to head (d01b5c8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6400      +/-   ##
==========================================
+ Coverage   49.30%   52.72%   +3.41%     
==========================================
  Files         146      125      -21     
  Lines       13708    12820     -888     
==========================================
  Hits         6759     6759              
+ Misses       6949     6061     -888     
Flag Coverage Δ
52.72% <ø> (+3.41%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rvandermeulen
Copy link
Contributor

dump_syms is a taskcluster toolchain artifact these days:
https://searchfox.org/mozilla-central/source/taskcluster/kinds/toolchain/dump-syms.yml

@mstange
Copy link

mstange commented Sep 27, 2024

Thank you, that was fast!

There have been two commits to this file on the application-services side, from PR #744 and PR #1453:
84e077d#diff-787f740bf8b1eb70e517c7c6441fb96645d3db9359f0017532ceaecf8cf92c3c
0db58fd

Do we need to reapply those? Probably hard to say without doing a full test run.

I do believe we need to update dump_syms, because the old version will probably complain if it's called with --inlines.

@bendk
Copy link
Contributor Author

bendk commented Sep 30, 2024

@mstange Looks like the first issue we're hitting is when importing buildconfig. How can we work around that? I think app-services could define our own buildconfig, but I'd need help for some of the attributes (DSYMUTIL, OTOOL, etc).

I also suspect once we figure that out, Python is going to complain about mozbuild. I see version 0.2 on pypi, can we use that?

@mstange
Copy link

mstange commented Oct 2, 2024

Sorry Ben, I don't actually know much about how this code and the python modules all work. That's unfortunate that copying the file wasn't enough to resolve this :(

dsymutil+otool are only used when compiling for macOS.
What platforms is libmegazord compiled for? Is it just Android? Or maybe also iOS?

@bendk
Copy link
Contributor Author

bendk commented Oct 2, 2024

dsymutil+otool are only used when compiling for macOS. What platforms is libmegazord compiled for? Is it just Android? Or maybe also iOS?

libmegazord is compiled for Android. libmegazord_ios and libmegazord_focus are used on iOS. It would be good to get symbols for those as well, but that can be a second pass.

@mstange
Copy link

mstange commented Oct 3, 2024

So I think there are two options:

  1. Either we can treat application-services' copy of symbolstore.py as a true fork, and manually apply the changes that happened to the m-c version of the file, or at least a subset of the changes that we care about.
  2. Or we do the necessary work so that the files can stay in sync. I don't have concrete proposals for how to do that.

Maybe we can do 1 for now, and leave 2 for the future. If there's a future where application-services becomes part of mozilla-central, then 2 would become easier at that point.

Is there anyone who might have opinions on the approach?

@bendk
Copy link
Contributor Author

bendk commented Oct 3, 2024

  1. makes sense to me as well. What are the changes we care about? I see this commit to add the --inlines argument, are there others?

@mstange
Copy link

mstange commented Oct 3, 2024

The --inlines change is the one I care the most about. And it requires an updated dump_syms version.

Additionally, the following would be nice to have, but are much less urgent:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants