-
Notifications
You must be signed in to change notification settings - Fork 894
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR description says that In my understanding of the PR description, I should be able to run For me, the only way this code has an effect is when the environment variable There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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 | ||
*/ | ||
|
@@ -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( | ||
|
@@ -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' | ||
|
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) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 agree it's nicer for local developers to have cli parameters to run against different output directories. As far as I know, the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I understand and agree; The priority is to unblock CI. We can do additional work in a follow-up.
In my opinion, the fact that Storybook has GN dependencies and itself needs
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", | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.