Skip to content

Cr110 rust #16789

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 10, 2023
Merged

Cr110 rust #16789

merged 1 commit into from
Feb 10, 2023

Conversation

bridiver
Copy link
Collaborator

@bridiver bridiver commented Jan 23, 2023 •
edited
Loading

Resolves brave/brave-browser#28446

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@bridiver bridiver requested review from a team January 23, 2023 02:04
@github-actions github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Jan 23, 2023
@bridiver bridiver requested a review from a team as a January 23, 2023 02:05
rillian
rillian approved these changes Jan 23, 2023
View reviewed changes
Copy link
Contributor

@rillian rillian left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice hack! It's worth taking this to upgrade freedom, if it works at all, until a more permanent fix is available upstream.

@rillian
Copy link
Contributor

rillian commented Jan 23, 2023 •
edited
Loading

CI failures are from a dirty Cargo.lock. Running locally it's reverting to cxx 1.0.81, so it's somehow finding the wrong version, even though ../third_party/rust/cxx/v1/README.chromium says 1.0.82.

This seems to be corrected now.

@rillian rillian mentioned this pull request Jan 23, 2023
Closed
@bridiver bridiver force-pushed the cr110-rust branch 2 times, most recently from 97a46d1 to 1c8a302 Compare January 23, 2023 23:53
@bridiver bridiver changed the base branch from cr110 to master January 23, 2023 23:53
@bridiver bridiver requested review from a team and simonhong as code owners January 23, 2023 23:58
@bridiver bridiver changed the base branch from master to cr110 January 23, 2023 23:59
@bridiver bridiver removed request for a team and simonhong January 23, 2023 23:59
@emerick emerick force-pushed the cr110 branch 4 times, most recently from 65d6afa to 5bd81ef Compare January 24, 2023 21:09
kylehickinson
kylehickinson approved these changes Jan 25, 2023
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS++

@bridiver bridiver force-pushed the cr110-rust branch 3 times, most recently from a62813a to 426d37a Compare January 25, 2023 18:37
mkarolin
mkarolin approved these changes Jan 26, 2023
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chromium_src and patches changes LGTM

Base automatically changed from cr110 to master January 26, 2023 14:12
rillian
rillian approved these changes Feb 10, 2023
View reviewed changes
build/rust/config.gni
@@ -13,6 +13,7 @@ import("//build/config/sysroot.gni")
declare_args() {
rustup_home = "//brave/build/rustup/$brave_rust_version"
cargo_profile = ""
enable_rust_lto = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't say what's wrong with lto. Should we be doing this for all platforms?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mac/ios was the only platform we enabled this for and it was specifically to deal with the "too many personalities" issue, but actually I wonder if we should be using rust LTO on other platforms to reduce binary size. Since they export so much stuff, it might not be getting stripped in the final output. macos/ios are ok because we use https://github.com/brave/brave-core/pull/16789/files#diff-84fabac7eb11280108df0ac0414a13e868865f07329ae57baff447d9b477c0deR20 to strip everything

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a separate issue from this PR though because it doesn't change anything for other platforms

@bridiver
Copy link
Collaborator Author

All platforms were green prior to squash and also verified on ios/android/macos configurations that don't run on PR builder

@bridiver bridiver merged commit d4976e7 into master Feb 10, 2023
@bridiver bridiver deleted the cr110-rust branch February 10, 2023 21:43
@github-actions github-actions bot added this to the 1.50.x - Nightly milestone Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rillian rillian rillian approved these changes

@kylehickinson kylehickinson kylehickinson approved these changes

@mkarolin mkarolin mkarolin approved these changes

@iefremov iefremov Awaiting requested review from iefremov

@goodov goodov Awaiting requested review from goodov

Assignees

@bridiver bridiver

Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps)
Projects
None yet
Milestone
1.50.x - Release
Development

Successfully merging this pull request may close these issues.

Upgrade rust to 1.67.0
4 participants
@bridiver @rillian @kylehickinson @mkarolin