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: Fix attribution generation #28415

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Nov 11, 2024

Description

Fix the attribution:generate command by ensuring that it is possible to install just production dependencies.

Previously the command yarn workspaces focus --production (used to discard development dependencies, keeping just production dependencies installed) would fail because rimraf was not found. rimraf was a development dependency used in the postinstall script. This was resolved by replacing rimraf with a Node.js script that does the same thing without needing any dependency.

Once that failure was resolved, another was revealed. The allow-scripts step of the installation began failing because there was a package detected that had an install script that was missing from our configuration. This package was in our configuration already, but the allow-scripts configuration is sensitive to changes in the directory structure of node_modules, and that structure changed due to differences in which packages were hoisted in the production-only install.

That failure was resolved by updating generate-attributions.sh to remove the allow-scripts plugin while generating attributions. We don't need postinstall scripts to run in order to read licences from disk.

Open in GitHub Codespaces

Related issues

Fixes #28412

Manual testing steps

  1. Run yarn attributions:generate, and see that it completes successfully
    • Locally, it should also re-install the allow-scripts plugin and development dependencies
    • If this command is run with CI=true (e.g. CI=true yarn attributions:generate), it will skip the step of re-installing the allow-scripts plugin and development dependencies. This is what would happen on CI, where the environment gets discarded after this is run so there is no point in re-installing things.

Screenshots/Recordings

N/A

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@@ -38,7 +45,9 @@ main() {

# Check if the script is running in a CI environment (GitHub Actions sets the CI variable to true)
if [ -z "${CI:-}" ]; then
# If not running in CI, restore development dependencies
# If not running in CI, restore the allow-scripts plugin and development dependencies.
cd "${PROJECT_DIRECTORY}"
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously the attempt to re-install development dependencies was also failing, because yarn was run in the wrong directory. This is now fixed as well.

Fix the `attribution:generate` command by ensuring that it is possible
to install just production dependencies.

Previously the command `yarn workspaces focus --production` (used to
discard development dependencies, keeping just production dependencies
installed) would fail because `rimraf` was not found. `rimraf` was a
development dependency used in the `postinstall` script. This was
resolved by replacing `rimraf` with a Node.js script that does the
same thing without needing any dependency.

Once that failure was resolved, another was revealed. The
`allow-scripts` step of the installation began failing because there
was a package detected that had an install script that was missing from
our configuration. This package was in our configuration already, but
the `allow-scripts` configuration is sensitive to changes in the
directory structure of `node_modules`, and that structure changed due
to differences in which packages were hoisted in the production-only
install.

That failure was resolved by updating `generate-attributions.sh` to
remove the `allow-scripts` plugin while generating attributions. We
don't need `postinstall` scripts to run in order to read licences from
disk.

Fixes #28412
@Gudahtt
Copy link
Member Author

Gudahtt commented Nov 11, 2024

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

@metamaskbot
Copy link
Collaborator

Builds ready [838989e]
Page Load Metrics (2019 ± 103 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26425441788613294
domContentLoaded16892469198219594
load170125262019214103
domInteractive199256199
backgroundConnect8128403417
firstReactRender491721083115
getState45113136
initialActions01000
loadScripts12301873146417584
setupStore56018199
uiStartup196528182249229110
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt marked this pull request as ready for review November 12, 2024 14:11
@Gudahtt Gudahtt requested review from a team and kumavis as code owners November 12, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

[Bug]: Attributions out-of-date
3 participants