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

Don't try to compile brave-rewards and brave-sync when disabled #4513

Closed
wants to merge 5 commits into from
Closed

Don't try to compile brave-rewards and brave-sync when disabled #4513

wants to merge 5 commits into from

Conversation

akshay24495
Copy link

@akshay24495 akshay24495 commented Feb 5, 2020

Fixes brave/brave-browser#8072

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@akshay24495 akshay24495 requested a review from bridiver as a code owner February 5, 2020 11:29
@akshay24495
Copy link
Author

I could not link the issue to the pull request ( brave/brave-browser#8072 (comment) )

@bsclifton
Copy link
Member

@bridiver @NejcZdovc @darkdh can you check this one out please? 😄 Changes LGTM

@NejcZdovc
Copy link
Contributor

@bsclifton this PR needs to be moved in our repo so that we can run CI on it

@bsclifton
Copy link
Member

Created #4720 so that CI can run 😄

@bsclifton
Copy link
Member

Successful CI run with #4720 🎉

Ready for review @NejcZdovc

@akshay24495
Copy link
Author

@NejcZdovc Kindly review

@akshay24495
Copy link
Author

@bsclifton Any updates?

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.

Changes LGTM++

#include "brave/components/brave_webtorrent/grit/brave_webtorrent_resources.h"

namespace extensions {

bool IsComponentExtensionWhitelisted(const std::string& extension_id) {
const char* const kAllowed[] = {
brave_extension_id,
#if BUILDFLAG(BRAVE_REWARDS_ENABLED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

preprocessor directives should not be indented

Copy link
Author

Choose a reason for hiding this comment

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

I have removed the indentation for preprocessor.

#include "brave/components/brave_sync/grit/brave_sync_resources.h"
#endif
#include "brave/components/brave_webtorrent/grit/brave_webtorrent_resources.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be moved above the conditionally added includes

Copy link
Author

Choose a reason for hiding this comment

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

I have moved the conditionally added includes

@bsclifton
Copy link
Member

Created #5124 so that we can let this run through our CI system 😄 @akshay24495 can you squash the commits up into one?

@bsclifton
Copy link
Member

CI passed with #5124 🎉 I can help you squash these down and will ping @bridiver for re-approval

browser/BUILD.gn Outdated
@@ -136,6 +136,8 @@ source_set("browser_process") {
"//brave/components/brave_webtorrent/browser/buildflags",
"//brave/components/content_settings/core/browser",
"//brave/components/greaselion/browser/buildflags",
"//brave/components/brave_rewards/browser/buildflags",
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry last thing, I know a bunch of these deps are already out of alphabetical order, but can you alphabetize them relative to this group of deps at least? https://github.com/brave/brave-core/pull/4513/files#diff-84f8700538180eec90bb2f4c9dff4da4R132
so "//brave/components/brave_rewards/browser/buildflags", would go after "//brave/components/brave_rewards/browser", and "//brave/components/brave_sync/buildflags", would go after "//brave/components/brave_shields/common",

Copy link
Collaborator

Choose a reason for hiding this comment

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

at least that way we won't be adding to the problem

Copy link
Author

Choose a reason for hiding this comment

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

Changes have been done

@akshay24495 akshay24495 requested a review from bridiver April 2, 2020 04:15
@NejcZdovc NejcZdovc removed this from the 1.8.x - Release milestone Apr 22, 2020
@NejcZdovc NejcZdovc removed their request for review June 17, 2020 07:30
@akshay24495 akshay24495 closed this Jun 7, 2021
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] Compiling brave rewards and brave sync when disabled in component extension whitelist
4 participants