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

chore: allow dependencies to be built with pnpm v10 #1448

Merged
merged 1 commit into from
Apr 7, 2025

Conversation

odockal
Copy link
Contributor

@odockal odockal commented Mar 18, 2025

What does this PR do?

Explicitly state the dependencies with build scripts to execute during ie. install lifecycle phase in pnpm v10. This will enable to e2e tests to run.

We have alternatives, I went with the minimal working solution, other options:

"onlyBuiltDependencies": ["electron", "@biomejs/biome", "esbuild", "svelte-preprocess"] -> building those that appeared on the warning
OR
"neverBuiltDependencies": [] -> to build any

When I run pnpm approve-builds locally, it updated pnpm-workspace.yaml file instead of package.json.

Screenshot / video of UI

What issues does this PR fix or reference?

#1447

How to test this PR?

E2E tests should be passing now.

@odockal odockal requested a review from a team as a code owner March 18, 2025 20:39
@odockal odockal requested review from jeffmaury, feloy, SoniaSandler, deboer-tim and benoitf and removed request for a team March 18, 2025 20:39
Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

electron should not be a 'known' dependency from the extension pov

@odockal
Copy link
Contributor Author

odockal commented Mar 26, 2025

Until it is resolved in the core Refactor the type of dependency for electron tests-playwright package, we cannot do much in the PR now.

@benoitf
Copy link
Contributor

benoitf commented Mar 26, 2025

@odockal

well you could add

If you want the old pre v10 behaviour, so you want to allow all dependencies to run postinstall scripts, then add this to your package.json:

{
  "pnpm": {
    "neverBuiltDependencies": []
  }
}

and it'll be like pnpm 9.x

package.json Outdated
@@ -7,7 +7,7 @@
"publisher": "redhat",
"private": true,
"engines": {
"node": ">=20.9.0",
"node": ">=22.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems unrelated

@odockal odockal force-pushed the add-electron-built-deps branch from d396726 to 34ecee2 Compare April 7, 2025 13:00
@odockal odockal force-pushed the add-electron-built-deps branch from 34ecee2 to 79e3bc7 Compare April 7, 2025 13:01
@odockal
Copy link
Contributor Author

odockal commented Apr 7, 2025

@odockal

well you could add

If you want the old pre v10 behaviour, so you want to allow all dependencies to run postinstall scripts, then add this to your package.json:

{
  "pnpm": {
    "neverBuiltDependencies": []
  }
}

and it'll be like pnpm 9.x

@benoitf sry, missed this comment.

@jeffmaury @deboer-tim Now it should be in a shape for review.

@benoitf benoitf enabled auto-merge (rebase) April 7, 2025 13:09
@benoitf benoitf changed the title Add electron built deps into package.json chore: allow dependencies to be built with pnpm v10 Apr 7, 2025
@benoitf benoitf merged commit cf4873c into podman-desktop:main Apr 7, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include Electron built dependencies into package.json upon move to pnpm v10
3 participants