Fix yarn workspaces on Windows #369
Conversation
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @jvff and @mvd-ows)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pronebird)
a discussion (no related file):
I ran into a problem and I'm not sure if it's related to this PR.
I tried running
cd gui
yarn install --force
cd ..
./build.sh --dev-build
and it failed in electron-builder, with the error:
Error: C:\Users\Janito\AppData\Local\electron-builder\Cache\nsis\nsis-3.0.3.2\Bin\makensis.exe exited with code 1
Output:
Command line defined: "APP_ID=net.mullvad.vpn"
Command line defined: "APP_GUID=8fa2c331-e09e-5709-bc74-c59df61f0c7e"
Command line defined: "UNINSTALL_APP_KEY=8fa2c331-e09e-5709-bc74-c59df61f0c7e"
Command line defined: "PRODUCT_NAME=Mullvad VPN"
Command line defined: "PRODUCT_FILENAME=Mullvad VPN"
Command line defined: "APP_FILENAME=Mullvad VPN"
Command line defined: "APP_DESCRIPTION=Mullvad VPN client"
Command line defined: "VERSION=2018.2.0-dev-503acc5e"
Command line defined: "PROJECT_DIR=C:\Users\Janito\Projects\mullvadvpn-app\gui\packages\desktop"
Command line defined: "BUILD_RESOURCES_DIR=C:\Users\Janito\Projects\mullvadvpn-app\dist-assets"
Command line defined: "MUI_ICON=C:\Users\Janito\Projects\mullvadvpn-app\dist-assets\icon.ico"
Command line defined: "MUI_UNICON=C:\Users\Janito\Projects\mullvadvpn-app\dist-assets\icon.ico"
Command line defined: "APP_64=C:\Users\Janito\Projects\mullvadvpn-app\dist\desktop-2018.2.0-dev-503acc5e-x64.nsis.7z"
Command line defined: "APP_64_NAME=desktop-2018.2.0-dev-503acc5e-x64.nsis.7z"
Command line defined: "APP_64_HASH=E279CA744D303F57FDBB7C875B67B4D369C122FBAE307A5740CA8B43D8F26CD7F7F911E4B2BA4162CFACE41A674C3BB2F0AB88BF0E59CA3F39E613642625BB89"
Command line defined: "COMPANY_NAME=Mullvad VPN"
Command line defined: "APP_INSTALLER_STORE_FILE=Mullvad VPN\__installer.exe"
Command line defined: "COMPRESSION_METHOD=7z"
Command line defined: "MULTIUSER_INSTALLMODE_ALLOW_ELEVATION"
Command line defined: "INSTALL_MODE_PER_ALL_USERS"
Command line defined: "INSTALL_MODE_PER_ALL_USERS_REQUIRED"
Command line defined: "allowToChangeInstallationDirectory"
Command line defined: "SHORTCUT_NAME=Mullvad VPN"
Command line defined: "UNINSTALL_DISPLAY_NAME=Mullvad VPN 2018.2.0-dev-503acc5e"
Command line defined: "MUI_WELCOMEFINISHPAGE_BITMAP=C:\Users\Janito\Projects\mullvadvpn-app\dist-assets\windows\installersidebar.bmp"
Command line defined: "MUI_UNWELCOMEFINISHPAGE_BITMAP=C:\Users\Janito\Projects\mullvadvpn-app\dist-assets\windows\installersidebar.bmp"
Command line defined: "ESTIMATED_SIZE=185982"
Command line defined: "COMPRESS=auto"
Command line defined: "UNINSTALLER_OUT_FILE=C:\Users\Janito\Projects\mullvadvpn-app\dist\.__uninstaller-nsis-desktop.exe"
Processing config: C:\Users\Janito\AppData\Local\electron-builder\Cache\nsis\nsis-3.0.3.2\nsisconf.nsh
Processing script file: "<stdin>" (ACP)
Error output:
File: "C:\Users\Janito\Projects\mullvadvpn-app\gui\packages\desktop\dist-assets\binaries\windows\driver\*" -> no files found.
Usage: File [/nonfatal] [/a] ([/r] [/x filespec [...]] filespec [...] |
/oname=outfile one_file_only)
Error in macro ExtractDriver on macroline 3
Error in macro customInstall on macroline 14
!include: error in script: "installSection.nsh" on line 76
Error in script "<stdin>" on line 150 -- aborting creation process
at ChildProcess.childProcess.once.code (C:\Users\Janito\Projects\mullvadvpn-app\gui\node_modules\builder-util\src\util.ts:254:14)
at Object.onceWrapper (events.js:317:30)
at emitTwo (events.js:126:13)
at ChildProcess.emit (events.js:214:7)
at maybeClose (internal/child_process.js:925:16)
at Process.ChildProcess._handle.onexit (internal/child_process.js:209:5)
|
a discussion (no related file): Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…
Good catch. The imports in the NSIS script are relative, and the new root seems to be @pronebird What do you think? |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jvff)
a discussion (no related file):
Previously, mvd-ows wrote…
Good catch. The imports in the NSIS script are relative, and the new root seems to be
C:\Users\Janito\Projects\mullvadvpn-app\gui\packages\desktop\, where the old one wasC:\Users\Janito\Projects\mullvadvpn-app\. Not sure if we should it in this PR or a separate one?@pronebird What do you think?
I can fix it as a part of this PR. Seems relevant to making yarn workspaces on Windows
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @mvd-ows and @jvff)
a discussion (no related file):
Previously, pronebird (Andrei Mihailov) wrote…
I can fix it as a part of this PR. Seems relevant to making yarn workspaces on Windows
I've fixed this now. Basically BUILD_RESOURCES_DIR points to dist-assets so there is no point in hardcoding the path so I used it instead where applicable.
Unfortunately I am not exactly sure how to sort out the path to C++ plugin for NSIS since PROJECT_DIR points to gui\packages\desktop.
So I simply used ${BUILD_RESOURCES_DIR}\..\windows\nsis-plugins\bin\Win32-Release which expands to {root-path}\dist-assets\..\windows\nsis-plugins\bin\Win32-Release. Not the most elegant way to fix this but many things about electron-builder are not elegant.
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @mvd-ows and @jvff)
a discussion (no related file):
Previously, pronebird (Andrei Mihailov) wrote…
I've fixed this now. Basically
BUILD_RESOURCES_DIRpoints todist-assetsso there is no point in hardcoding the path so I used it instead where applicable.Unfortunately I am not exactly sure how to sort out the path to C++ plugin for NSIS since
PROJECT_DIRpoints togui\packages\desktop.So I simply used
${BUILD_RESOURCES_DIR}\..\windows\nsis-plugins\bin\Win32-Releasewhich expands to{root-path}\dist-assets\..\windows\nsis-plugins\bin\Win32-Release. Not the most elegant way to fix this but many things about electron-builder are not elegant.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @mvd-ows and @jvff)
pronebird
force-pushed
the
fix-yarn-workspaces-windows
branch
from
d2ce81f
to
d6a8835
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on @mvd-ows and @jvff)
pronebird
force-pushed
the
fix-yarn-workspaces-windows
branch
from
d6a8835
to
dcee2f0
Compare
There was a problem hiding this comment.
Updated electron-builder to the latest but seems like nothing new in there 😕
Reviewable status: 2 of 6 files reviewed, all discussions resolved (waiting on @mvd-ows and @jvff)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: 2 of 6 files reviewed, all discussions resolved (waiting on @mvd-ows and @jvff)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: 4 of 6 files reviewed, all discussions resolved (waiting on @mvd-ows and @jvff)
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @mvd-ows, @jvff, and @pronebird)
gui/packages/yarn-fixes/patch-yarn.js, line 3 at r4 (raw file):
// Yarn 1.9.4 has a path lookup bug on Windows, when it looks for the binaries referenced in // scripts under '\gui\node_modules\node_modules' instead of '\gui\node_modules'. // This patch adds a junction between those two to keep that house of cards from falling apart.
Is this bug a reported issue? If so, could we link to that issue in this comment? So it's easier to detect/follow up when it's fixed so this workaround can be removed.
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @mvd-ows, @jvff, and @pronebird)
gui/packages/yarn-fixes/patch-yarn.js, line 3 at r4 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Is this bug a reported issue? If so, could we link to that issue in this comment? So it's easier to detect/follow up when it's fixed so this workaround can be removed.
If you go all the way up and read "WHY" section to this PR, you'll find the issue link on Github too.
I"ll dupe it in the source code comment too.
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @mvd-ows, @jvff, and @faern)
gui/packages/yarn-fixes/patch-yarn.js, line 3 at r4 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
If you go all the way up and read "WHY" section to this PR, you'll find the issue link on Github too.
I"ll dupe it in the source code comment too.
Yes please. I would not consider it a dupe. Because in one weeks time no one will look back at the PR comments, only the code. And if it's hard to check if the issue has been resolved I'm afraid the workaround will live forever.
pronebird
force-pushed
the
fix-yarn-workspaces-windows
branch
from
dcee2f0
to
75d94b5
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @mvd-ows, @jvff, and @faern)
gui/packages/yarn-fixes/patch-yarn.js, line 3 at r4 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Yes please. I would not consider it a dupe. Because in one weeks time no one will look back at the PR comments, only the code. And if it's hard to check if the issue has been resolved I'm afraid the workaround will live forever.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @mvd-ows, @jvff, and @faern)
There was a problem hiding this comment.
Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on @mvd-ows and @jvff)
Describe what this PR changes. Why this is wanted. And, if needed, how it does it.
Git checklist:
CHANGELOG.mdunder the[Unreleased]header.WHY
For some reason Yarn on Windows looks for binaries, referenced in
scriptssection ofpackage.json, ingui/node_modules/node_modulesdirectory instead ofgui/node_modules. (Issue: yarnpkg/yarn#4564)HOW
This PR adds a package with no source code that performs a preinstall script that creates a junction between
gui/node_modules/node_modulesandgui/node_modules.Other things I've tried before coming up with this solution:
Adding the same script to preinstall step in root
package.json- it didn't work because yarn removed the junction at some point later.Using BAT file and package.json bounded to
os: win32. Didn't work either because yarn refuses to proceed with installation on other OS. Corresponding issue: Yarn workspaces with multi-platform packages yarnpkg/yarn#6288Please try this patch on your windows machine, after pulling the PR's branch, please run:
If something still goes wrong, please run:
And submit
yarn.logto me in private chat on Slack. Thanks.ALSO
This PR fixes the similar issue related to wiring react-native on Windows. Unfortunately creating a symlink on Windows requires elevated permissions, but junctions are available for low privilege users and work just fine.
This change is