-
Notifications
You must be signed in to change notification settings - Fork 36
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(build): fix package on windows & cmd #298
Conversation
@@ -10,7 +10,7 @@ const installBindings = async () => { | |||
|
|||
for (const platform of platforms) { | |||
await execute( | |||
`./node_modules/.bin/node-pre-gyp install --directory=./node_modules/sqlite3 --target_platform=${platform} --target_arch=x64`, | |||
`cross-env ./node_modules/.bin/node-pre-gyp install --directory=./node_modules/sqlite3 --target_platform=${platform} --target_arch=x64`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure that calling cross-env like this requires having it installed globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since we don´t mention those bindings in the assets of pkg (in the main package.json), I think we use the bindings provided by the native-extensions package instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I don't have it installed globally. Dot commands doesn't work on windows. Using cross-env is the only safe option to run commands across all OS
@@ -11,7 +11,7 @@ | |||
"clean": "yarn workspaces foreach -pi run clean", | |||
"start": "ts-node -T --dir scripts start_studio", | |||
"setup": "ts-node -T --dir scripts create_default_env --location ./.env.debug", | |||
"package": "yarn clean && yarn build && ts-node -T --dir scripts package", | |||
"package": "ts-node -T --dir scripts package", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the clean and build part here? It makes more sense to remove the old dist and then re-build the project to be always sure the resulting bundle only contains the right files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because when I type the command "package", I expect it to just package what is there. If we need to clean and build beforehand, then we can type those commands in succession. Typing a command shouldn't be a "surprise". When you build, it builds, when you clean, it cleans.
We added gulp in the main repository to handle special commands like yarn cmd build:core
so we can add commands without denaturing the "official" commands (when you yarn build, it builds everything, and we keep the scripts list small)
Packaging failed on windows because of the command not including cross-env.
Also made a small adjustment to the package.json file, scripts should do a single task... Package shouldn't clean/build/package, it just packages. If those commands must be used together, then they must be typed independently