-
Notifications
You must be signed in to change notification settings - Fork 895
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
Add missing brave_generated_resources_grit deps #2440
Conversation
Still seeing the following on Windows in the built PR:
|
bd8860a
to
3259777
Compare
3259777
to
9d3b453
Compare
4485bb5
to
4d2197b
Compare
4d2197b
to
543be3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix should be correct, per discussion with @petemill 😄👍
https://bravesoftware.slack.com/archives/CA5FPHWLF/p1557822903090000
@@ -39,6 +39,12 @@ source_set("net") { | |||
"//net", | |||
] | |||
|
|||
if (!is_android) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can android app not use strings from //brave/app:brave_generated_resources_grit
@simonhong @bridiver ? If not, and //brave/browser/net
will be used by android build, how do we get those string resources declared for android - do they need to be added separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of these guards were just to get the build working. I didn't have time to dig into each one so @SergeyZhukovsky is working on fixing actual runtime issues related to these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most likely we need to split these grd file up for things like extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petemill what was the build error on android?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question re: android, but this fixes the missing dependencies at least for desktop builds
Add missing brave_generated_resources_grit deps
Add missing brave_generated_resources_grit deps
Fix brave/brave-browser#4428
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.