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

Fix build dependency issue with settings and extension pages resources #7599

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

petemill
Copy link
Member

@petemill petemill commented Jan 14, 2021

Resolves brave/brave-browser#13567

This should be uplifted to wherever chromium 88 is

Submitter Checklist:

  • There is a ticket for my issue.
  • Used Github auto-closing keywords in the commit message.
  • Wrote a good PR/commit description
  • 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, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed).
  • Requested a security/privacy review as needed.

Reviewer Checklist:

  • 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:

@petemill petemill requested a review from a team as a code owner January 14, 2021 07:08
@petemill petemill self-assigned this Jan 14, 2021
"../../../../ui/webui/resources:preprocess",
]
excludes = [ "chrome://resources/js/cr.m.js" ]
+ deps += brave_deps_chrome_browser_resources_extensions_build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

@bsclifton bsclifton Jan 14, 2021

Choose a reason for hiding this comment

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

I believe we do need the other patch as line 17 in patches/chrome-browser-resources-settings-BUILD.gn.patch is used for our settings customizations. This is a different page
cc: @petemill

Copy link
Member Author

@petemill petemill Jan 14, 2021

Choose a reason for hiding this comment

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

@bridiver I believe we do as they are different sets of files used at different steps of the build, and we can't simply add these new ones to that other brave target because it still won't guarantee it will happen in the right order, and we can't add the deps only in the other place as that's inside a optimize_webui conditional.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bsclifton same applies to both settings and extensions. They both need 2 patches as far as I can tell so far.

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

approving to unbreak the build, but I'd like to follow-up and make sure we have only the patches we need and not any extra

@petemill
Copy link
Member Author

re-building macos

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

++

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.

++

@petemill
Copy link
Member Author

Merging because CI is only being problematic for macOS and that's build-instance related, not PR-related. We've tested ourselves with macOS, all the other platforms have built ok with CI and this is only a dep addition aimed to fix CI.

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.

[Desktop] Build can fail with settings and extension pages dependency issue
5 participants