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.

Sign up for GitHub

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

Jump to bottom

Switch npm to pnpm #714

Merged
merged 1 commit into from Nov 25, 2022
Merged

Switch npm to pnpm #714

merged 1 commit into from Nov 25, 2022

Conversation

balajiv113
Copy link
Contributor

Pre-flight Checklist

  1. Please remember that if you are logging a bug for some service that has stopped working or is working incorrectly, please log the bug here
  2. If you are requesting support for a new service in Ferdium, please log it here
  3. Please remember to read the self-help documentation - in case it helps you unblock yourself for issues related to older versions of recipes that were installed on your machine. (These will get automatically upgraded when you upgrade to the newer versions of Ferdium, but to get new recipes between Ferdium releases, this documentation is quite useful.)
  4. Please ensure you've completed all of the following.

Description of Change

Move electron-builder outside of package.json script. As using inside is causing trouble in native buildings. The same model we already followed to fix windows building with pnpm

Checklist

  • My pull request is properly named
  • The changes respect the code style of the project (pnpm prepare-code)
  • pnpm test passes
  • I tested/previewed my changes locally

Testing

I have done testing on macOS

Steps to test

  • Remove --$TARGET_ARCH --$TARGET_OS --dir from the build scripts
  • Run CLEAN=true ./scripts/build-unix.sh
  • Once done, copy arm Ferdi.app to M1 machine and verify if no error thrown in console and vice versa (Build in arm and copy amd Ferdi.app to intel and verify no errors thrown)

@balajiv113 balajiv113 requested a review from a team as a code owner October 27, 2022 07:28
@balajiv113 balajiv113 marked this pull request as draft October 28, 2022 13:43
@balajiv113
Copy link
Contributor Author

balajiv113 commented Oct 28, 2022 •
edited by vraravam

electron-builder.npmrc tweaks

This file contains tweaks required to run electron-builder with pnpm mentioned here electron-userland/electron-builder#6289 (comment)
Note electron-builder.npmrc will be renamed to .npmrc and move inside build folder for electron-builder build phase

Issues after adding tweaks

  1. Windows github action build issue (Not a valid 32bit application), related to pnpm monorepo build error electron-userland/electron-builder#6933.
    Note: Current tweak of running pnpm electron-builder directly (not using package.json scripts) is wrong. With this electron-builder is using npm and not pnpm
  2. Pnpm not rebuilding native binaries, related to pnpm rebuild not working when node-linker=hoisted pnpm/pnpm#5560

Core issue
Above tweaks will not needed if this issue is fixed develar/app-builder#84

Tests to be done
The below check should be done when migrating to pnpm again

  • local & github actions builds are fine on all OS & platform
  • Cross platfrom checks for macOS - Build both binaries (amd64 & arm64) on intel machine and see if arm64 binaries are running fine on mac M1 and vice versa

@balajiv113 balajiv113 changed the title fix issue with pnpm and electron-builder in nightly Switch npm to pnpm Oct 28, 2022
@balajiv113 balajiv113 marked this pull request as ready for review November 13, 2022 15:54
@balajiv113
Copy link
Contributor Author

Moving back to draft :) As it is again blocked

@balajiv113 balajiv113 marked this pull request as draft November 14, 2022 13:17
@balajiv113 balajiv113 marked this pull request as ready for review November 23, 2022 12:48
@balajiv113
Copy link
Contributor Author

Added yet another small tweak to get around the blocker.

@vraravam vraravam force-pushed the fix-pnpm branch 3 times, most recently from 9045efe to a82917f Compare November 24, 2022 05:46
SpecialAro
SpecialAro approved these changes Nov 24, 2022
View reviewed changes
Copy link
Member

@SpecialAro SpecialAro left a comment •
edited

Choose a reason for hiding this comment

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

I wasn't able to build this PR locally because it was throwing some errors related with MSVS Tools (I tested 2022, 2019 and 2017 with no success - it doesn't fail for my arch x64, but fails for the others).

Nevertheless, as the CI didn't fail for Windows, I vote for we to move to merge this PR.

Thank you @balajiv113 for not giving up on this conversion 😄

@Alphrag
Copy link
Member

Alphrag commented Nov 24, 2022 •
edited

Tested to build on my windows 10 x64 and the builds for all architecture work exactly as expected! Well done @balajiv113 for that work 🤩 🎉

pnpm_build_pass

@balajiv113 @nathanaelhoun
@SpecialAro @vraravam
Switch from 'npm' to 'pnpm'
c523310 
Co-authored-by: Markus Hatvan <markus_hatvan@aon.at>
Co-authored-by: Nathanaël Houn <contact@nathanaelhoun.fr>
Vijay Aravamudhan <vraravam@users.noreply.github.com>
Co-authored-by: André Oliveira <oliveira.andrerodrigues95@gmail.com>
@vraravam vraravam merged commit c04b71b into ferdium:develop Nov 25, 2022
@balajiv113 balajiv113 deleted the fix-pnpm branch November 25, 2022 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alphrag Alphrag Alphrag approved these changes

@SpecialAro SpecialAro SpecialAro approved these changes

Assignees

@balajiv113 balajiv113

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@balajiv113 @Alphrag @SpecialAro @vraravam