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 update branding logic to make release builds faster #16033

Merged
merged 3 commits into from
Nov 22, 2022

Conversation

goodov
Copy link
Member

@goodov goodov commented Nov 19, 2022

Currently MacOS tests in release branches fallback to development branding, which is incorrect.
This happens because config.channel is development in tests. Regression from: #13597

This PR:

  1. disables updateBranding() call for npm run test.
  2. adds util.touchOverriddenFiles() to create_dist command as it's used as a default build command in Android PR builds. This should be a no-op in most scenarios. If it's not, then we'd better fix those scenarios.
  3. fixes unnecessary BRANDING, app.icns rewrites when --channel is passed.

Resolves brave/brave-browser#26869

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, npm run lint, 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:

@goodov goodov requested a review from mkarolin November 19, 2022 05:58
@goodov goodov changed the title Test command shouldn't update branding. Don't update branding in npm run test command. Nov 19, 2022
@goodov goodov force-pushed the dont-update-branding-in-tests branch from 22dbb3a to 3f7f9e9 Compare November 19, 2022 06:13
@goodov goodov force-pushed the dont-update-branding-in-tests branch from 5341f4d to 67940dc Compare November 19, 2022 06:52
console.log('copy branding file')
fs.copySync(brandingSource, brandingDest)
}
}
Copy link
Member Author

@goodov goodov Nov 19, 2022

Choose a reason for hiding this comment

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

this was always rewriting BRANDING and icns files, which triggered unnecessary rebuilds during create_dist in non-development channels.

the logic was:

  1. rewrite all files using fileMap by comparing timestamps/checksums, which always restored "development" files.
  2. rewrite BRANDING and icns on top, using config.channel value.

now we use channel-specific files without unnecessary overwrites.

Copy link
Collaborator

Choose a reason for hiding this comment

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

running npm run test ... without first running npm run build ... fails now

ninja: Entering directory `/Volumes/brave/src/out/Component'
ninja: error: '../../components/vector_icons/brave/product_refresh.icon', needed by 'gen/components/vector_icons/vector_icons.cc', missing and no known rule to make it

Copy link
Member Author

@goodov goodov Jun 5, 2024

Choose a reason for hiding this comment

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

running npm run test ... without first running npm run build ... fails now

ninja: Entering directory `/Volumes/brave/src/out/Component'
ninja: error: '../../components/vector_icons/brave/product_refresh.icon', needed by 'gen/components/vector_icons/vector_icons.cc', missing and no known rule to make it

yes, that was always the case. npm run test also never calls gn gen, so you had to call npm run build at least once to prepare everything. I usually do npm run build -- --target=base before running some test on a fresh checkout.

to fix this we need to have the right branding dependency done via GN, hopefully this task will align what we have in the right direction: brave/brave-browser#38309

@goodov goodov changed the title Don't update branding in npm run test command. Fix update branding logic to make builds faster Nov 19, 2022
@goodov goodov changed the title Fix update branding logic to make builds faster Fix update branding logic to make release builds faster Nov 19, 2022
@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 67940dc
reason: unsigned
Please follow the handbook to configure commit signing
cc: @goodov

@goodov goodov merged commit 0fb3e90 into master Nov 22, 2022
@goodov goodov deleted the dont-update-branding-in-tests branch November 22, 2022 08:26
@github-actions github-actions bot added this to the 1.47.x - Nightly milestone Nov 22, 2022
// Set proper mac app icon for channel to chrome/app/theme/mac/app.icns.
// Each channel's app icons are stored in brave/app/theme/$channel/app.icns.
// With this copying, we don't need to modify chrome/BUILD.gn for this.
const iconSource = path.join(braveAppDir, 'theme', 'brave', 'mac', config.channel, 'app.icns')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why aren't we just copying all channels here?

Copy link
Member Author

@goodov goodov Jun 5, 2024

Choose a reason for hiding this comment

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

source file depends on the channel, but the destination file does not.


// Set proper branding file.
let branding_file_name = 'BRANDING'
if (config.channel)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above, we should always copy all the files or this is not accurate #16033 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

if we remove the channel dependency here then we can update the branding at sync time and only update on build when needed

Copy link
Member Author

Choose a reason for hiding this comment

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

same here. The destination file is a single file, it doesn't depend on the channel, so we can't copy "all the files" into the same destination file..

I have a task to rework this: brave/brave-browser#38309

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.

Tests on MacOS use development branding instead of the channel branding
2 participants