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

Fix(Cordova) enhanced node path detection, debug symbols now run in foreground. #694

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix(Cordova) enhanced node path detection, debug symbols now run in foreground. #694

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97663e5
improve cordova setup
lucas-zimerman Oct 22, 2024
93dd7eb
changelog and fix
lucas-zimerman Oct 22, 2024
51a9cc2
remove unused field
lucas-zimerman Oct 22, 2024
f99ae89
Merge branch 'master' into fix/cordova-node
lucas-zimerman Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Changelog

## Unreleased

- fix(cordova): Don't fail to build if node isn't found. ([#694](https://github.com/getsentry/sentry-wizard/pull/694))
- ref(cordova): Improved logs with Cordova integration and Sentry-CLI now runs in foregroung. ([#694](https://github.com/getsentry/sentry-wizard/pull/694))

## 3.34.2

- fix(nextjs): Don't ask for package manager twice (#690)
Expand Down
79 changes: 58 additions & 21 deletions lib/Steps/Integrations/Cordova.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export class Cordova extends BaseIntegration {
return;
}
}
const cwd = path.join(process.cwd(), 'sentry.properties');
path.join(process.cwd(), 'sentry.properties');
proj.addBuildPhase(
[],
'PBXShellScriptBuildPhase',
Expand All @@ -190,31 +190,68 @@ export class Cordova extends BaseIntegration {
shellPath: '/bin/sh',
shellScript:
// eslint-disable-next-line prefer-template
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to a shell file that will ship with wizard and then copy the content to create the build phase?

For example lib/Steps/Integrations/Cordova/SentryUploadDebugFiles.sh?

'#!/bin/bash\\n' +
'# print commands before executing them and stop on first error\\n' +
'set -x -e\\n' +
'echo "warning: uploading debug symbols - set SENTRY_SKIP_DSYM_UPLOAD=true to skip this"\\n' +
'if [ -n "$SENTRY_SKIP_DSYM_UPLOAD" ]; then\\n' +
' echo "warning: skipping debug symbol upload"\\n' +
' exit 0\\n' +
'fi\\n' +
'export SENTRY_PROPERTIES=' +
cwd +
'\\n' +
'function getProperty {\\n' +
' PROP_KEY=$1\\n' +
' PROP_VALUE=`cat $SENTRY_PROPERTIES | grep "$PROP_KEY" | cut -d\'=\' -f2`\\n' +
' if [ -z "$PROP_VALUE" ]; then\\n' +
' echo "plugins/sentry-cordova/node_modules/@sentry/cli/bin/sentry-cli"\\n' +
' else\\n' +
' echo $PROP_VALUE\\n' +
'[ -z "$SENTRY_FORCE_FOREGROUND"] && SENTRY_FORCE_FOREGROUND=true\\n' +
'[[ "$SENTRY_FORCE_FOREGROUND" == true ]] && SENTRY_FORCE_FOREGROUND_FLAG="--force-foreground"\\n' +
Comment on lines +197 to +198
Copy link
Member

@krystofwoldrich krystofwoldrich Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since https://github.com/getsentry/sentry-cli/releases/tag/2.37.0 the upload is always in foreground. So I think we can just bump the CLI shipped with cordova and leave the flag out.

'get_node_command() {\\n' +
' if [ -x "$(command -v ${NODE_BINARY})" ]; then\\n' +
' echo "${NODE_BINARY}"\\n' +
' return 0\\n' +
' fi\\n' +
' if [ -x "$(which node)" ]; then\\n' +
' echo "node"\\n' +
' return 0\\n' +
' fi\\n' +
' NVM_NODE_VERSIONS_DIR="$HOME/.nvm/versions/node"\\n' +
' if [ -d "$NVM_NODE_VERSIONS_DIR" ] && [ "$(ls -A $NVM_NODE_VERSIONS_DIR)" ]; then\\n' +
' HIGHEST_VERSION=$(ls -v "$NVM_NODE_VERSIONS_DIR" | tail -n 1)\\n' +
' NODE_BINARY="$NVM_NODE_VERSIONS_DIR/$HIGHEST_VERSION/bin/node"\\n' +
' if [ -x "$NODE_BINARY" ]; then\\n' +
' echo "$NODE_BINARY"\\n' +
' return 0\\n' +
' fi\\n' +
' fi\\n' +
' echo ""\\n' +
' return 0\\n' +
'}\\n' +
Comment on lines +199 to 219
Copy link
Member

Choose a reason for hiding this comment

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

Are we getting complains about the node not being set?
I think we should just allow to use NODE_BINARY if global node is not set.
We can make sure that the NODE_BINARY by the wizard.

For example RN deprecated the dynamic node lookup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I experienced it where node wasn't found by Xcode while it was found by the terminal

'if [ ! -f $SENTRY_PROPERTIES ]; then\\n' +
' echo "warning: SENTRY: sentry.properties file not found! Skipping symbol upload."\\n' +
'LOCAL_NODE_BINARY=$(get_node_command)\\n' +
'if [ -z "$LOCAL_NODE_BINARY" ]; then\\n' +
' echo "warning: SENTRY: Node.js binary not found! Skipping symbol upload."\\n' +
' exit 0\\n' +
'else\\n' +
' echo "Using Node.js from ${LOCAL_NODE_BINARY}"\\n' +
'fi\\n' +
'if [ -z "$SENTRY_PROPERTIES" ]; then\\n' +
' if [ -f "./sentry.properties" ]; then\\n' +
' export SENTRY_PROPERTIES=sentry.properties\\n' +
' elif [ -f "../../sentry.properties" ]; then\\n' +
' export SENTRY_PROPERTIES=../../sentry.properties\\n' +
' else\\n' +
' echo "warning: SENTRY: sentry.properties file not found! Skipping symbol upload."\\n' +
' exit 0\\n' +
' fi\\n' +
'fi\\n' +
'echo "# Reading property from $SENTRY_PROPERTIES"\\n' +
'SENTRY_CLI=$(getProperty "cli.executable")\\n' +
'SENTRY_COMMAND="../../$SENTRY_CLI upload-dsym"\\n' +
'$SENTRY_COMMAND',
'echo "sentry properties found at : $(readlink -f ${SENTRY_PROPERTIES})"\\n' +
'[ -z "$SENTRY_CLI_EXECUTABLE" ] && SENTRY_CLI_PACKAGE_PATH=$("$LOCAL_NODE_BINARY" --print "require(\'path\').dirname(require.resolve(\'@sentry/cli/package.json\'))")\\n' +
'[ -z "$SENTRY_CLI_EXECUTABLE" ] && SENTRY_CLI_EXECUTABLE="${SENTRY_CLI_PACKAGE_PATH}/bin/sentry-cli"\\n' +
'SENTRY_COMMAND="${SENTRY_CLI_EXECUTABLE} upload-dsym $SENTRY_FORCE_FOREGROUND_FLAG"\\n' +
'if [ "$SENTRY_SKIP_DSYM_UPLOAD" != true ]; then\\n' +
' set +x +e\\n' +
' SENTRY_XCODE_COMMAND_OUTPUT=$(/bin/sh -c "$LOCAL_NODE_BINARY $SENTRY_COMMAND" 2>&1)\\n' +
' if [ $? -eq 0 ]; then\\n' +
' echo "$SENTRY_XCODE_COMMAND_OUTPUT"\\n' +
' echo "$SENTRY_XCODE_COMMAND_OUTPUT" | awk \'{print "output: sentry-cli - " $0}\'\\n' +
' else\\n' +
' echo "error: sentry-cli - To disable debug symbols upload, set SENTRY_SKIP_DSYM_UPLOAD=true in your environment variables. Or to allow failing upload, set SENTRY_ALLOW_FAILURE=true"\\n' +
' echo "error: sentry-cli - $SENTRY_XCODE_COMMAND_OUTPUT"\\n' +
' fi\\n' +
' set -x -e\\n' +
'else\\n' +
' echo "SENTRY_SKIP_DSYM_UPLOAD=true, skipping debug symbols upload"\\n' +
'fi',
},
);
}
Expand Down
Loading