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(package): set process.env.npm_config_arch for each arch in universal build #3884

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

Conversation

busterbogheart
Copy link

@busterbogheart busterbogheart commented Mar 13, 2025

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Closes #3657

This one line will set process.env.npm_config_arch to the appropriate architecture as the build creates multiple versions for the Mac universal app.

@busterbogheart busterbogheart requested a review from a team as a code owner March 13, 2025 15:38
@erickzhao erickzhao changed the title fix(osxUniversal): set appropriate archs in process.env (issue #3657) fix(universal): set appropriate archs in process.env (issue #3657) Mar 13, 2025
@erickzhao erickzhao changed the title fix(universal): set appropriate archs in process.env (issue #3657) fix(universal): set appropriate archs in process.env Mar 13, 2025
@erickzhao erickzhao changed the title fix(universal): set appropriate archs in process.env fix(package): set process.env.npm_config_arch for each arch in universal build Mar 13, 2025
@erickzhao erickzhao self-requested a review March 13, 2025 21:30
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

Hey @busterbogheart,

Thanks for the PR on this and sorry for the lack of response on the original issue.

This seems reasonable to me, but I pushed up a new commit to be more intentional about setting this env var specifically for packaging universal apps: c012573

Can you let me know if this works for you?

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Hey @busterbogheart can you provide a sample basic electron app that has the problem you're describing that is fixed by the fix you're proposing.

Not entirely sure how this fixes things, and having that repro would potentially allow us to find a more safe solution.

@Dexus
Copy link

Dexus commented Mar 31, 2025

@MarshallOfSound

its anoying to workaround all this shit, the patch here, is working semi good. Maybe for some of the packages, but there ist still more todo...

use the package: @deepnest/svg-preprocessor it has optional packages each for different platforms and architectures.

Same problem also for prebuilds from prebuildify like @deepnest/calculate-nfp

both should be able to load for universal builds, but don't. And I'm currently not sure, what I can do, to let it build correctly. Currently I'm try this: https://github.com/Dexus/Deepnest/blob/9a1cd7bb1c1829037f5445a4c48e7bb333a8e6b5/forge.config.js#L61-L133

@MarshallOfSound
Copy link
Member

Same comment as above, gesturing wildly at random packages and saying "this doesn't work" is not helpful.

The maintainers have limited bandwidth, if you want someone to look at a thing seriously, provide a repro case. A tiny repo (not a giant app) that demonstrates the problem you are reporting. If you do, it's much more likely people will be able to help you.

@Dexus
Copy link

Dexus commented Mar 31, 2025

Checkout this REPO: https://github.com/Dexus/Deepnest.git and then checkout to 9e2181aac5dc950d3d0fac4e836e3f81f17073e0 run and get your errors.

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.

using electron/universal, some modules do not rebuild according to arch
4 participants