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

petemill · 2023-12-21T10:37:39Z

Plus, generate tsconfig file for even more specific path mapping.

Target / target arch / etc can be explicitly provided by environment variables (or the usual brave-core .npmrc / .env params):

config.update({
  target_arch: process.env.TARGET_ARCH,
  target_os: process.env.TARGET_OS,
  target_environment: process.env.TARGET_ENVIRONMENT,
  target: process.env.TARGET
})

Devops PR that provides these variables in CI https://github.com/brave/devops/pull/11412

But failing that, it will make a good guess based on the most recently built target directory (looking at modified time). Previous to this PR it did the same but only for the gen subdirectory, which seems to be less accurate.

This should satisfy all use cases (the lazy local dev and the explicit CI environment).

However, annother added change on this PR is to start with config's default output directory based on the current architecture.

Resolves brave/brave-browser#34984

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
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 wiki
- npm run lint, npm run presubmit wiki, 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:

.storybook/webpack.config.js

mherrmann · 2023-12-21T13:49:54Z

package.json

@@ -36,7 +36,7 @@
    "pep8": "pycodestyle --max-line-length 120 -r script",
    "pylint": "node ./build/commands/scripts/commands.js pylint",
    "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",


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.

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.

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.

mherrmann · 2023-12-21T13:51:56Z

.storybook/webpack.config.js

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


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).

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.

:+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.

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.

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

mherrmann · 2023-12-21T13:56:48Z

Also, don't we need this change as well?

mherrmann · 2023-12-21T13:59:25Z

(I heard that the problem also occurred on Arm64 macOS, so I added the associated CI label.)

petemill · 2023-12-21T18:51:33Z

Also, don't we need this change as well?

Yes all generated JS used by any stories should be referenced there.

fallaciousreasoning

LGTM @petemill - thanks for doing this!

fallaciousreasoning · 2023-12-21T19:56:19Z

.storybook/webpack.config.js

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


:+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.

mherrmann

I have quality concerns (described in comments) but am unlocking this PR so @petemill can go ahead and unblock CI.

@petemill if you don't have time to fix this issue, let me know and I will try to create a fix.

mherrmann · 2024-01-31T07:43:15Z

.storybook/webpack.config.js

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


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.

mherrmann · 2024-01-31T07:49:28Z

.storybook/webpack.config.js

+// 3. most recently built - this caters to the common scenario when a
+// non-standard target has been built but no arguments are provided to storybook
+config.update({
+  target_arch: process.env.TARGET_ARCH,


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.

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.

.storybook/webpack.config.js.orig

mherrmann · 2024-01-31T09:31:45Z

package.json

@@ -36,7 +36,7 @@
    "pep8": "pycodestyle --max-line-length 120 -r script",
    "pylint": "node ./build/commands/scripts/commands.js pylint",
    "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",


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.

mherrmann · 2024-01-31T09:47:14Z

.storybook/webpack.config.js

+    .filter(a => fs.existsSync(a))
+    .sort((a, b) => fs.statSync(b).mtime - fs.statSync(a).mtime)


@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.

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

.storybook/webpack.config.js

mherrmann · 2024-01-31T09:48:37Z

.storybook/BUILD.gn

@@ -19,6 +19,7 @@ group("storybook") {
    "//brave/components/brave_news/common:mojom_js",
    "//brave/components/brave_shields/common:mojom_js",
    "//brave/components/brave_vpn/common/mojom:mojom_js",
+    "//brave/components/brave_rewards/common/mojom:mojom_js",


Do we maybe also need the following here?

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

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

mihaiplesa · 2024-02-20T11:58:22Z

Our PR builds on arm64 are still red until we fix this. Can we find a way to get it in @petemill?

kdenhartog

unblocking js-deps-reviewers. There's no additional dependency changes happening so approving. Added one question as well.

kdenhartog · 2024-02-22T22:42:54Z

.storybook/webpack.config.js

+// 3. most recently built - this caters to the common scenario when a
+// non-standard target has been built but no arguments are provided to storybook
+config.update({
+  target_arch: process.env.TARGET_ARCH,


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.

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

goodov

this is not perfect and IMO should be solved in a way when we set the build directory explicitly on CI.

but right now to unblock existing pipelines this approach should do just fine.

.storybook/webpack.config.js.orig

goodov

pls ignore this comment. accidental leftover.

build/commands/lib/genTsConfig.js

components/webpack/BUILD.gn

…specification via param or fallback to most recently built Plus, generate tsconfig file for even more specific path mapping Ensure rewards mojom is built before storybook

mihaiplesa · 2024-03-14T16:21:18Z

SB_BUILDER-WEBPACK5_0002 (WebpackInvocationError): Module not found: Error: Can't resolve './.storybook/chrome-resources-mock/js/assert.js' in './components/brave_wallet_ui/common/hooks' is the only left error, on macOS arm64.

…scripts

mihaiplesa · 2024-03-14T21:09:29Z

All green now.

petemill · 2024-03-21T04:57:01Z

Unless anyone objects, then I'm going to merge this as it unblocks certain CI builds. It doesn't introduce the unpopular
"detection" of the build directory for local developers - it keeps what we currently have on master. It adds an additional way to override the target arch via environment variables. In a follow-up we'd likely keep this, but have those variables passed from GN / Python, and consider using GN to run storybook both in watch/web-server mode (local developers) and build-storybook mode (CI).

edit: ...once I fix npm run storybook which is now broken...

mherrmann · 2024-03-21T07:56:10Z

It doesn't introduce the unpopular "detection" of the build directory for local developers

If I understand this correctly, then it is about the following:

const outDirectories = getBuildOutputPathList()
    .filter(a => fs.existsSync(a))
    .sort((a, b) => fs.statSync(b).mtime - fs.statSync(a).mtime)
outputPath = outDirectories[0]

As far as I see, that code is still part of the PR. Can we remove it?

petemill · 2024-03-21T17:17:24Z

@mherrmann That is not introduced in this PR. It is currently on master. It enables all the frontend developers to run npm run storybook. Can we please propose changing that in a subsequent PR?

All this PR does is introduce an override for that output path lookup, which can be provided via the environment variables.

mherrmann · 2024-03-21T17:22:18Z

@petemill ah yes, sorry. I only saw the code in the diff as "added"; I did not see that it was removed by the diff as well.

…specification via param or fallback to most recently built (uplift to 1.65.x) (#22733) Uplift of #21443 (squashed) to beta

…specification via param or fallback to most recently built (uplift to 1.64.x) (#22734) Uplift of #21443 (squashed) to release

petemill requested review from mherrmann and fallaciousreasoning December 21, 2023 10:37

petemill self-assigned this Dec 21, 2023

petemill requested review from a team and bridiver as code owners December 21, 2023 10:37

github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Dec 21, 2023

github-actions bot reviewed Dec 21, 2023

View reviewed changes

.storybook/webpack.config.js Show resolved Hide resolved

.storybook/webpack.config.js Show resolved Hide resolved

github-actions bot added the needs-security-review label Dec 21, 2023

github-actions bot assigned fmarier and thypon Dec 21, 2023

petemill added CI/storybook-url Deploy storybook and provide a unique URL for each build CI/run-linux-arm64 Run CI builds for Linux arm64 and removed needs-security-review labels Dec 21, 2023

mherrmann requested changes Dec 21, 2023

View reviewed changes

mherrmann added the unused-CI/run-macos-arm64 Run CI builds for macOS arm64 label Dec 21, 2023

fmarier unassigned fmarier and thypon Dec 21, 2023

fallaciousreasoning approved these changes Dec 21, 2023

View reviewed changes

petemill force-pushed the storybook-target-param branch from ed9f916 to d34e65c Compare December 22, 2023 03:33

github-actions bot removed the CI/storybook-url Deploy storybook and provide a unique URL for each build label Dec 22, 2023

petemill force-pushed the storybook-target-param branch from d34e65c to 028b844 Compare December 27, 2023 01:31

petemill force-pushed the storybook-target-param branch from 028b844 to e3044da Compare January 31, 2024 07:19

mherrmann approved these changes Jan 31, 2024

View reviewed changes

kdenhartog approved these changes Feb 22, 2024

View reviewed changes

petemill force-pushed the storybook-target-param branch from e3044da to a3a6f2f Compare February 26, 2024 03:02

petemill requested a review from a team as a code owner February 26, 2024 03:02

mihaiplesa requested a review from goodov February 29, 2024 14:47

goodov approved these changes Feb 29, 2024

View reviewed changes

goodov reviewed Feb 29, 2024

View reviewed changes

.storybook/webpack.config.js.orig Outdated Show resolved Hide resolved

goodov reviewed Feb 29, 2024

View reviewed changes

build/commands/lib/genTsConfig.js Outdated Show resolved Hide resolved

components/webpack/BUILD.gn Outdated Show resolved Hide resolved

bridiver mentioned this pull request Mar 12, 2024

move rust crates to chromium_crate_io #21759

Merged

24 tasks

petemill force-pushed the storybook-target-param branch 2 times, most recently from 4a16455 to cb29d93 Compare March 14, 2024 05:20

Storybook - better determine correct brave-core output path allowing …

8e2b0a8

…specification via param or fallback to most recently built Plus, generate tsconfig file for even more specific path mapping Ensure rewards mojom is built before storybook

petemill force-pushed the storybook-target-param branch from cb29d93 to 8e2b0a8 Compare March 14, 2024 05:24

mihaiplesa added the CI/run-windows-arm64 Run CI builds for Windows arm64 label Mar 14, 2024

Allow build config to be customized via Config.update during command …

54ccb2f

…scripts

mherrmann approved these changes Mar 21, 2024

View reviewed changes

petemill merged commit 1663573 into master Mar 21, 2024
19 checks passed

petemill deleted the storybook-target-param branch March 21, 2024 17:33

github-actions bot added this to the 1.66.x - Nightly milestone Mar 21, 2024

mihaiplesa mentioned this pull request Mar 21, 2024

Fix the "storybook" stage of CI PR builds for Arm64 Linux & macOS #21432

Closed

25 tasks

fallaciousreasoning mentioned this pull request Mar 21, 2024

[Storybook]: Typecheck in another process for non-CI builds #22669

Merged

24 tasks

brave-builds added a commit that referenced this pull request Mar 22, 2024

Uplift of #21443 (squashed) to beta

b458359

brave-builds added a commit that referenced this pull request Mar 22, 2024

Uplift of #21443 (squashed) to release

878a013

kjozwiak pushed a commit that referenced this pull request Mar 22, 2024

Storybook - better determine correct brave-core output path allowing …

6a9c2ff

…specification via param or fallback to most recently built (uplift to 1.65.x) (#22733) Uplift of #21443 (squashed) to beta

kjozwiak pushed a commit that referenced this pull request Mar 22, 2024

Storybook - better determine correct brave-core output path allowing …

93acb3e

…specification via param or fallback to most recently built (uplift to 1.64.x) (#22734) Uplift of #21443 (squashed) to release

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petemill commented Dec 21, 2023 •

edited

Loading

mherrmann Dec 21, 2023

petemill Dec 21, 2023 •

edited

Loading

mherrmann Jan 31, 2024

mherrmann Dec 21, 2023

petemill Dec 21, 2023 •

edited

Loading

fallaciousreasoning Dec 21, 2023

mherrmann Jan 31, 2024

bridiver Mar 19, 2024

mherrmann commented Dec 21, 2023

mherrmann commented Dec 21, 2023

petemill commented Dec 21, 2023

fallaciousreasoning left a comment

fallaciousreasoning Dec 21, 2023

mherrmann left a comment

mherrmann Jan 31, 2024

mherrmann Jan 31, 2024

petemill Feb 26, 2024

mherrmann Jan 31, 2024

mherrmann Jan 31, 2024

petemill Feb 26, 2024

mherrmann Jan 31, 2024

petemill Feb 26, 2024

mihaiplesa commented Feb 20, 2024 •

edited

Loading

kdenhartog left a comment

kdenhartog Feb 22, 2024

petemill Feb 22, 2024

goodov left a comment •

edited

Loading

goodov left a comment •

edited

Loading

mihaiplesa commented Mar 14, 2024

mihaiplesa commented Mar 14, 2024

petemill commented Mar 21, 2024 •

edited

Loading

mherrmann commented Mar 21, 2024

petemill commented Mar 21, 2024

mherrmann commented Mar 21, 2024

		.filter(a => fs.existsSync(a))
		.sort((a, b) => fs.statSync(b).mtime - fs.statSync(a).mtime)

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

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

Conversation

petemill commented Dec 21, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

petemill Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mherrmann commented Dec 21, 2023

mherrmann commented Dec 21, 2023

petemill commented Dec 21, 2023

fallaciousreasoning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mherrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaiplesa commented Feb 20, 2024 • edited Loading

kdenhartog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodov left a comment • edited Loading

Choose a reason for hiding this comment

goodov left a comment • edited Loading

Choose a reason for hiding this comment

mihaiplesa commented Mar 14, 2024

mihaiplesa commented Mar 14, 2024

petemill commented Mar 21, 2024 • edited Loading

mherrmann commented Mar 21, 2024

petemill commented Mar 21, 2024

mherrmann commented Mar 21, 2024

petemill commented Dec 21, 2023 •

edited

Loading

petemill Dec 21, 2023 •

edited

Loading

petemill Dec 21, 2023 •

edited

Loading

mihaiplesa commented Feb 20, 2024 •

edited

Loading

goodov left a comment •

edited

Loading

goodov left a comment •

edited

Loading

petemill commented Mar 21, 2024 •

edited

Loading