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

Storybook - better determine correct brave-core output path allowing specification via param or fallback to most recently built #21443

Merged
merged 2 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .storybook/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ group("storybook") {
deps = [
"//brave/components/brave_new_tab_ui:mojom_js",
"//brave/components/brave_news/common:mojom_js",
"//brave/components/brave_rewards/common/mojom:mojom_js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we maybe also need the following here?

"//brave/components/playlist/common/mojom:mojom_js",

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps, but that has assert(enable_playlist) and it's more controversial to remove, so we can do as follow-up.

"//brave/components/brave_shields/core/common:mojom_js",
"//brave/components/brave_vpn/common/mojom:mojom_js",
"//brave/components/brave_wallet/common:mojom_js",
Expand Down
67 changes: 48 additions & 19 deletions .storybook/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,34 +6,68 @@
const path = require('path')
const webpack = require('webpack')
const fs = require('fs')
const config = require('../build/commands/lib/config')
const genTsConfig = require('../build/commands/lib/genTsConfig')
const {
fallback,
provideNodeGlobals
} = require('../components/webpack/polyfill')
const generatePathMap = require('../components/webpack/path-map')

const buildConfigs = ['Component', 'Static', 'Debug', 'Release']
const extraArchitectures = ['arm64', 'x86']

function getBuildOuptutPathList(buildOutputRelativePath) {
// Choose which brave-core build directory to look for pre-compiled
// resource dependencies:
// 1. Default for local builds for the actual platform / architecture
// 2. platform / architecture overriden by environment variables
// 3. most recently built - this caters to the common scenario when a
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would personally not implement the "most recently built" logic. It is ambiguous and has already caused problems for me. Yes, that was with the gen subdirectory; But the principle remains the same and I strongly suspect that there will be a day where it causes a confusing surprise for another developer (directly or through CI).

Copy link
Member Author

@petemill petemill Dec 21, 2023

Choose a reason for hiding this comment

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

I'm keeping it for now to provide backwards compatibility. Since this is 99% a local dev debug and development preview tool, the risk is minimal, and in fact the chances of getting it wrong is improved.

Copy link
Contributor

Choose a reason for hiding this comment

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

:+1 I'm pretty happy to keep this in - I think it'll work most of the time and you can make things explicit if you need to now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still disagree with this, but it seems I am outvoted for now. Maybe @bridiver (whose approval is also required for this PR, as far as I can tell) will agree with me. If not, feel free to mark this comment of mine as resolved @petemill.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have some concerns about this PR as well @mherrmann and discussing with @petemill

// non-standard target has been built but no arguments are provided to storybook.

// This uses environment variables as there is currently no way to pass custom
// arguments to the |storybook build| cli.
config.update({
target_arch: process.env.TARGET_ARCH,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The PR description says that target_arch etc. can be provided by the usual brave-core .npmrc / .env params. I'm not sure this section of the code achieves this.

In my understanding of the PR description, I should be able to run npm config set target_arch arm64 or to add target_arch=arm64 to my .env file. But the former is no longer supported (it gives error "target_arch is not a valid npm option"). And the latter has no effect.

For me, the only way this code has an effect is when the environment variable TARGET_ARCH is set, or defined in the same upper-case spelling in .env. I can't think of a situation in practice where a developer will do this. Maybe I'm missing something and there are real-world situations where CI or a dev will do this. But unless that is realistically the case, I would remove this code because it will effectively be unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

IT will support whatever we currently support for our other commands. If more recent versions of npm don't allow custom params then you must use what we already support. What most devs and CI use is CLI params to npm run sync and npm run build. However, Storybook doesn't support custom arguments, so for this command we specify via environment param instead.

Copy link
Member

Choose a reason for hiding this comment

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

Would this work as expected in a parallels VM or VirtualBox VM? From what I remember they normal replicate the architecture information from the host, so I don't think it would be a problem, but wanted to double check if you've thought about that at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this would be an issue separate to the main build command but either way this PR allows the user to override with cli, which was not possible before

target_os: process.env.TARGET_OS,
target_environment: process.env.TARGET_ENVIRONMENT,
target: process.env.TARGET,
build_config: process.env.BUILD_CONFIG
})

let outputPath = config.outputDir

function getBuildOutputPathList() {
return buildConfigs.flatMap((config) => [
petemill marked this conversation as resolved.
Show resolved Hide resolved
petemill marked this conversation as resolved.
Show resolved Hide resolved
path.resolve(__dirname, `../../out/${config}/${buildOutputRelativePath}`),
path.resolve(__dirname, `../../out/${config}`),
petemill marked this conversation as resolved.
Show resolved Hide resolved
...extraArchitectures.map((arch) =>
path.resolve(
__dirname,
`../../out/${config}_${arch}/${buildOutputRelativePath}`
`../../out/${config}_${arch}`
petemill marked this conversation as resolved.
Show resolved Hide resolved
)
)
])
}

const genFolder = getBuildOuptutPathList('gen')
.filter((a) => fs.existsSync(a))
.sort((a, b) => fs.statSync(b).mtime - fs.statSync(a).mtime)[0]
if (!genFolder) {
throw new Error('Failed to find build output folder!')
if (fs.existsSync(outputPath)) {
console.log('Assuming precompiled dependencies can be found at the existing path found from brave-core configuration: ' + outputPath)
} else {
const outDirectories = getBuildOutputPathList()
.filter(a => fs.existsSync(a))
.sort((a, b) => fs.statSync(b).mtime - fs.statSync(a).mtime)
Comment on lines +55 to +56
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bridiver this is the section of code I disagree with; I believe its goal is to pick the last-modified out/ directory (out/Component, out/Release, etc.) as the directory to use for the build.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm maintaining current functionality and adding the new functionality on top for now. We can remove in a follow-up.

if (!outDirectories.length) {
throw new Error('Cannot find any brave-core build output directories. Have you run a brave-core build yet with the specified (or default) configuration?')
}
outputPath = outDirectories[0]
}

const basePathMap = require('../components/webpack/path-map')(genFolder)
const genPath = path.join(outputPath, 'gen')

if (!fs.existsSync(genPath)) {
throw new Error("Failed to find build output 'gen' folder! Have you run a brave-core build yet with the specified (or default) configuration?")
}
console.log(`Using brave-core generated dependency path of '${genPath}'`)

const basePathMap = generatePathMap(genPath)

// Override the path map we use in the browser with some additional mock
// directories, so that we can replace things in Storybook.
Expand All @@ -52,12 +86,8 @@ const pathMap = {
* support for scheme prefixes (like chrome://)
*
* Note: This prefixReplacer is slightly different from the one we use in proper
* builds, as it takes the first match from any build folder, rather than
* specifying one - we don't know what the user built last, so we just see what
* we can find.
* builds, as some path maps have multiple possible locations (e.g. for mocks).
*
* This isn't perfect, and in future it'd be good to pass the build folder in
* via an environment variable. For now though, this works well.
* @param {string} prefix The prefix
* @param {string[] | string} replacements The real path options
*/
Expand Down Expand Up @@ -116,6 +146,9 @@ function useMockedModules(moduleNames) {

// Export a function. Accept the base config as the only param.
module.exports = async ({ config, mode }) => {
const tsConfigPath = await genTsConfig(genPath, 'tsconfig-storybook.json', genPath, path.resolve(__dirname, '../tsconfig-storybook.json'))
console.log(`Using generated tsconfig path of '${tsConfigPath}'`)

const isDevMode = mode === 'development'
// Make whatever fine-grained changes you need
config.module.rules.push(
Expand Down Expand Up @@ -148,11 +181,7 @@ module.exports = async ({ config, mode }) => {
test: /\.(ts|tsx)$/,
loader: 'ts-loader',
options: {
// TODO(petemill): point to the tsconfig in gen/[target] that
// is made during build-time, or generate a new one. For both those
// options, use a cli arg or environment variable to obtain the correct
// build target.
configFile: path.resolve(__dirname, '..', 'tsconfig-storybook.json'),
configFile: tsConfigPath,
getCustomTransformers: path.join(
__dirname,
'../components/webpack/webpack-ts-transformers.js'
Expand Down
4 changes: 4 additions & 0 deletions build/commands/lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,10 @@ Config.prototype.update = function (options) {
this.targetEnvironment = options.target_environment
}

if (options.build_config) {
this.buildConfig = options.build_config
}

if (options.is_asan) {
this.is_asan = true
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,30 @@
// Copyright (c) 2023 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

const fs = require('fs-extra')
const path = require('path')
const pathMap = require('./path-map')(process.env.ROOT_GEN_DIR)

const srcPath = path.resolve(__dirname, '../../../')
const braveSrcPath = path.join(srcPath, 'brave')
const Config = require('./config')

/**
* Generates a tsconfig.json file in the gen/ directory
* so that typescript can import files from cthe current build's
* gen/ directory (e.g. mojom-generated JS).
*
* @param {*} [atPath=process.env.ROOT_GEN_DIR]
* @returns void
* @param {*} genPath precompiled brave-core gen dir full path
* @param {*} name name of tsconfig file, e.g. tsconfig-webpack.json
* @param {*} atPath where to generate the file
* @param {*} extendsFrom full path of tsconfig to extend
* @returns full path to created tsconfig file
*/
async function createGenTsConfig (atPath = process.env.ROOT_GEN_DIR) {
module.exports = async function createGenTsConfig (genPath = process.env.ROOT_GEN_DIR, name = 'tsconfig-webpack.json', atPath = genPath, extendsFrom = path.join(Config.braveCoreDir, 'tsconfig-webpack.json')) {
const pathMap = require('../../../components/webpack/path-map')(genPath)
const configExtendsFrom = path.relative(
atPath,
path.join(braveSrcPath, 'tsconfig-webpack.json')
extendsFrom
)
const tsConfigPath = path.join(atPath, 'tsconfig-webpack.json')
const tsConfigPath = path.join(atPath, name)
// Even though ts-loader will get the paths from webpack for module resolution
// that does not help some issues where chromium both generates ts definitions
// and has JSDoc comments for the .m.js file. Sometimes the JSDoc is incorrect
Expand All @@ -41,16 +47,10 @@ const braveSrcPath = path.join(srcPath, 'brave')
references: [
{
// This ts project is generated by //ui/webui/resources:library
path: path.join(atPath, 'ui/webui/resources/tsconfig.json')
path: path.join(genPath, 'ui/webui/resources/tsconfig.json')
}
]
}
await fs.writeFile(tsConfigPath, JSON.stringify(config))
return tsConfigPath
}

createGenTsConfig()
.catch(err => {
console.error(err)
process.exit(1)
})
12 changes: 12 additions & 0 deletions build/commands/scripts/genTsConfig.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Copyright (c) 2023 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

const genTsConfig = require('../lib/genTsConfig')

genTsConfig()
.catch(err => {
console.error(err)
process.exit(1)
})
5 changes: 4 additions & 1 deletion components/webpack/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ action("generate_tsconfig") {
"//ui/webui/resources/js:build_ts",
]
inputs = [
"gen-tsconfig.js",
"//brave/package.json",
"//brave/build/commands/lib/config.js",
"//brave/build/commands/lib/genTsConfig.js",
"//brave/build/commands/scripts/genTsConfig.js",
"path-map.js",
]
outputs = [ "$root_gen_dir/tsconfig-webpack.json" ]
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"eslint": "eslint ./components",
"pep8": "pycodestyle --max-line-length 120 -r script",
"web-ui-gen-grd": "node components/webpack/gen-webpack-grd",
"web-ui-gen-tsconfig": "node components/webpack/gen-tsconfig",
"web-ui-gen-tsconfig": "node build/commands/scripts/genTsConfig",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make web-ui-gen-tsconfig and build-storybook take the target_arch, target_os andtarget_environment as parameters, which then get forwarded via the environment variables or some other way? We have npm run build Component so npm run build-storybook Component makes intuitive sense. Similarly for npm run build -- Static --target_arch=arm64 and target_os.

Actually, in my opinion, it seems like the storybook should be a GN target. Then we would automatically get all the target_arch etc. logic for free. But it may be beyond the scope of this PR.

Copy link
Member Author

@petemill petemill Dec 21, 2023

Choose a reason for hiding this comment

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

There is a storybook GN target which ensures the dependencies are built.
I do not think running storybook via GN is necessarily the best or obvious approach. Storybook is not an artifact of brave-core but a tool that uses the artifacts from brave-core as 1 of its dependencies. It creates a long running web server with a watch on all dependencies.

I do agree it's nicer for local developers to have cli parameters to run against different output directories. As far as I know, the storybook and build-storybook commands do not expose custom cli parameters to their configurations. We would have to write wrapper commands to accept cli parameters. I used environment variables for now since this is a CI-only issue and CI needs to be unblocked without waiting for someone to have the time to change everything about how we use storybook. It can be done as a follow-up when someone has available time. For now, the same .env file variables will be respected.

web-ui-gen-tsconfig is already only internally run from GN. It's also not called from this storybook config, only the browser build. I just refactored it so it can be called as a function. It doesn't need to accept parameters because it isn't run directly by the user or CI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

CI needs to be unblocked without waiting for someone to have the time to change everything about how we use storybook. It can be done as a follow-up

I understand and agree; The priority is to unblock CI. We can do additional work in a follow-up.

Storybook is not an artifact of brave-core but a tool that uses the artifacts from brave-core as 1 of its dependencies.

In my opinion, the fact that Storybook has GN dependencies and itself needs target_os etc. indicates to me that GN is a natural place for it to be called from.

this is a CI-only issue

I also experienced the issue during local development, so it's not a CI-only issue in my opinion.

"web-ui": "webpack --config components/webpack/webpack.config.js",
"build-storybook": "npm run build -- --target=brave:storybook && storybook build -c .storybook -o .storybook-out",
"storybook": "storybook dev",
Expand Down
Loading