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.

Sign up for GitHub

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
Merged

Fix update branding logic to make release builds faster #16033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a93194
Test command shouldn't update branding.
goodov Nov 19, 2022
3f7f9e9
Touch overridden files in create_dist command.
goodov Nov 19, 2022
67940dc
Don't replace BRANDING files twice.
goodov Nov 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion build/commands/lib/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ const build = (buildConfig = config.defaultBuildConfig, options) => {
config.update(options)
checkVersionsMatch()

util.touchOverriddenFilesAndUpdateBranding()
util.touchOverriddenFiles()
util.updateBranding()

if (config.xcode_gen_target) {
util.generateXcodeWorkspace()
Expand Down
1 change: 1 addition & 0 deletions build/commands/lib/createDist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const fs = require('fs-extra')
const createDist = (buildConfig = config.defaultBuildConfig, options) => {
config.buildConfig = buildConfig
config.update(options)
util.touchOverriddenFiles()
util.updateBranding()
// On Android CI does two builds sequentially: for aab and for apk.
// Symbols are uploaded after 2nd build, but we need to preserve the symbols
Expand Down
2 changes: 1 addition & 1 deletion build/commands/lib/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ const test = (passthroughArgs, suite, buildConfig = config.defaultBuildConfig, o
} else {
config.buildTarget = suite
}
util.touchOverriddenFilesAndUpdateBranding()
util.touchOverriddenFiles()
util.buildTarget()

// Filter out upstream tests that are known to fail for Brave
Expand Down
55 changes: 24 additions & 31 deletions build/commands/lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,24 @@ const util = {
fileMap.add([path.join(config.braveCoreDir, 'ui', 'webui', 'resources', 'css', 'text_defaults_md.css'),
path.join(config.srcDir, 'ui', 'webui', 'resources', 'css', 'text_defaults_md.css')])

let explicitSourceFiles = new Set()
if (process.platform === 'darwin') {
// 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 •
edited
Loading

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.

const iconDest = path.join(chromeAppDir, 'theme', 'brave', 'mac', 'app.icns')
explicitSourceFiles[iconDest] = iconSource

// 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

branding_file_name = branding_file_name + '.' + config.channel
const brandingSource = path.join(braveAppDir, 'theme', 'brave', branding_file_name)
const brandingDest = path.join(chromeAppDir, 'theme', 'brave', 'BRANDING')
explicitSourceFiles[brandingDest] = brandingSource
}

for (const [source, output] of fileMap) {
if (!fs.existsSync(source)) {
console.warn(`Warning: The following file-system entry was not found for copying contents to a chromium destination: ${source}. Consider removing the entry from the file-map, or investigating whether the correct source code reference is checked out.`)
Expand All @@ -267,8 +285,9 @@ const util = {
sourceFiles = [source]
}

for (const sourceFile of sourceFiles) {
let destinationFile = path.join(output, path.relative(source, sourceFile))
for (let sourceFile of sourceFiles) {
const destinationFile = path.join(output, path.relative(source, sourceFile))
sourceFile = explicitSourceFiles[destinationFile] || sourceFile
if (!fs.existsSync(destinationFile) ||
util.calculateFileChecksum(sourceFile) != util.calculateFileChecksum(destinationFile)) {
fs.copySync(sourceFile, destinationFile)
Expand All @@ -277,31 +296,6 @@ const util = {
}
}

if (process.platform === 'darwin') {
// Copy 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')
const iconDest = path.join(chromeAppDir, 'theme', 'brave', 'mac', 'app.icns')
if (!fs.existsSync(iconDest) ||
util.calculateFileChecksum(iconSource) != util.calculateFileChecksum(iconDest)) {
console.log('copy app icon')
fs.copySync(iconSource, iconDest)
}

// Copy branding file
let branding_file_name = 'BRANDING'
if (config.channel)
branding_file_name = branding_file_name + '.' + config.channel

const brandingSource = path.join(braveAppDir, 'theme', 'brave', branding_file_name)
const brandingDest = path.join(chromeAppDir, 'theme', 'brave', 'BRANDING')
if (!fs.existsSync(brandingDest) ||
util.calculateFileChecksum(brandingSource) != util.calculateFileChecksum(brandingDest)) {
console.log('copy branding file')
fs.copySync(brandingSource, brandingDest)
}
}
Copy link
Member Author

@goodov goodov Nov 19, 2022 •
edited
Loading

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 •
edited
Loading

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

if (config.targetOS === 'android') {

let braveOverwrittenFiles = new Set();
Expand Down Expand Up @@ -405,7 +399,7 @@ const util = {
}
},

touchOverriddenFiles: () => {
touchOverriddenChromiumSrcFiles: () => {
console.log('touch original files overridden by chromium_src...')

// Return true when original file of |file| should be touched.
Expand Down Expand Up @@ -465,10 +459,9 @@ const util = {
})
},

touchOverriddenFilesAndUpdateBranding: () => {
util.touchOverriddenFiles()
touchOverriddenFiles: () => {
util.touchOverriddenChromiumSrcFiles()
util.touchOverriddenVectorIconFiles()
util.updateBranding()
},

// Chromium compares pre-installed midl files and generated midl files from IDL during the build to check integrity.
Expand Down