Skip to content

/ brave-core

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

Make mini installer for non-stable channel work properly #214

Merged
merged 1 commit into from Jul 3, 2018
Merged

Make mini installer for non-stable channel work properly #214

merged 1 commit into from Jul 3, 2018

Conversation

@simonhong
Copy link
Collaborator

simonhong commented Jul 2, 2018 •
edited

It was not installed properly because needed string was empty.
Also, brave_strings.grd is added to generate_strings's source list to trigger re-generation.

With this PR, mini_installer.exe installs all non-stable channels well.

Issue: brave/brave-browser#396

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.

Test Plan:

mini_installer.exe --chrome-beta for beta channel.
mini_installer.exe --chrome-dev for dev channel.
mini_installer.exe --chrome-sxs for nightly channel.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
@simonhong simonhong self-assigned this Jul 2, 2018
@simonhong simonhong requested a review from bbondy Jul 2, 2018
@simonhong simonhong force-pushed the make_window_silent_installer_work branch from a9b3b04 to 593a5cc Jul 2, 2018
@bridiver
bridiver reviewed Jul 2, 2018
View changes
patches/chrome-installer-util-BUILD.gn.patch Outdated
--- a/chrome/installer/util/BUILD.gn
+++ b/chrome/installer/util/BUILD.gn
@@ -246,9 +246,16 @@ action("generate_strings") {
@@ -246,9 +246,22 @@ action("generate_strings") {
"$target_gen_dir/installer_util_strings.rc",
]

+ brand = "$branding_path_component"

This comment has been minimized.

Sign in to view
Copy link
@bridiver

bridiver Jul 2, 2018

Collaborator

Why are we changing the branding_path_component here?

This comment has been minimized.

Sign in to view
Copy link
@simonhong

simonhong Jul 2, 2018

Author Collaborator

This change is not introduced in this PR.
brave/brave-browser#153 tracks this.

This comment has been minimized.

Sign in to view
Copy link
@bridiver

bridiver Jul 2, 2018

Collaborator

sorry, you're right. I'm going to take a look at that issue
@bridiver
bridiver reviewed Jul 2, 2018
View changes
patches/chrome-installer-util-BUILD.gn.patch
+ # TODO(shong): Remove this.
+ brand = brand + "-development"
+ if (brave_chromium_build) {
+ # When brave_strings.grd is modified, outputs should be re-generated.

This comment has been minimized.

Sign in to view
Copy link
@bridiver

bridiver Jul 2, 2018

Collaborator

this script uses chromium_strings.grd as the grd file so why are we adding brave_strings.grd as a source?

This comment has been minimized.

Sign in to view
Copy link
@simonhong

simonhong Jul 2, 2018 •
edited

Author Collaborator

//chrome/installer/util::generate_strings uses create_string_rc.py and that script uses brave_strings.grd.

This comment has been minimized.

Sign in to view
Copy link
@bridiver

bridiver Jul 2, 2018

Collaborator

That seems like a strange way to handle it. Can we mark this whole thing as a temporary workaround and not just the -development part? Open a new issue?

This comment has been minimized.

Sign in to view
Copy link
@simonhong

simonhong Jul 2, 2018

Author Collaborator

brave_strings.grd is not related with brand. What do you mean?
Also, you think our changes in create_string_rc.py should be changed too?

This comment has been minimized.

Sign in to view
Copy link
@bridiver

bridiver Jul 2, 2018

Collaborator

I'm saying that putting brave_strings.grd inside create_string_rc.py doesn't seem like the best way to handle this and that's what I'd like to open another ticket for.

This comment has been minimized.

Sign in to view
Copy link
@simonhong

simonhong Jul 2, 2018

Author Collaborator

I see. I'll create new issue for that.

This comment has been minimized.

Sign in to view
Copy link
@simonhong

simonhong Jul 2, 2018

Author Collaborator

Added more comment and brave/brave-browser#472 is created for this.

This comment has been minimized.

Sign in to view
Copy link
@simonhong

simonhong Jul 3, 2018

Author Collaborator

Do you have more comment on this PR?

This comment has been minimized.

Sign in to view
Copy link
@bridiver

bridiver Jul 3, 2018

Collaborator

sorry, no. I'll approve

This comment has been minimized.

Sign in to view
Copy link
@simonhong

simonhong Jul 3, 2018

Author Collaborator

Thanks for kind review!
@simonhong
Make mini installer for non-stable channel work properly
256f5aa 
It was not installed properly because needed string was empty.
@simonhong simonhong force-pushed the make_window_silent_installer_work branch from 593a5cc to 256f5aa Jul 2, 2018
@bridiver
bridiver approved these changes Jul 3, 2018
View changes
@simonhong simonhong merged commit 08ddb80 into master Jul 3, 2018
@simonhong simonhong deleted the make_window_silent_installer_work branch Jul 3, 2018
@simonhong simonhong mentioned this pull request Jul 3, 2018
Closed
12 of 12 tasks complete
@bbondy
bbondy reviewed Jul 13, 2018
View changes
app/brave_strings.grd
@@ -137,11 +137,14 @@ If you update this file, be sure also to update google_chrome_strings.grd. -->
Brave
</message>
<if expr="is_win">
<message name="IDS_SXS_SHORTCUT_NAME" desc="Unused in Brave builds" translateable="false">

This comment has been minimized.

Sign in to view
Copy link
@bbondy

bbondy Jul 13, 2018

Member

These should be in the rebaes_l10n script since this file is generated.

This comment has been minimized.

Sign in to view
Copy link
@bbondy

bbondy Jul 13, 2018

Member

I'll handle it, but for future reference we never want to modify .grd files in patches
https://github.com/brave/brave-browser/blob/master/lib/l10nUtil.js#L87
@simonhong simonhong mentioned this pull request Jul 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbondy bbondy

@bridiver bridiver

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@simonhong @bridiver @bbondy
You can’t perform that action at this time.