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(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

Conversation

lucas-zimerman
Copy link
Contributor

This is a refactor on how the Cordova Integration runs, the main changes on the PR are:

  • More path options for sentry.properties.
  • Use the latest version of nvm node as a fallback if no node versions were found.
  • Skip the integration if node isn't found, avoiding a build failure when running Sentry-CLI.
  • Use a more modern approach to find where Sentry-CLI is located.
  • Run Sentry/CLI in foreground to help with debug information logs.

@lucas-zimerman lucas-zimerman marked this pull request as ready for review October 22, 2024 17:17
@lucas-zimerman
Copy link
Contributor Author

formatted code:

#!/bin/bash
# print commands before executing them and stop on first error
set -x -e

echo "warning: uploading debug symbols - set SENTRY_SKIP_DSYM_UPLOAD=true to skip this"

[ -z "$SENTRY_FORCE_FOREGROUND"] && SENTRY_FORCE_FOREGROUND=true
[[ "$SENTRY_FORCE_FOREGROUND" == true ]] && SENTRY_FORCE_FOREGROUND_FLAG="--force-foreground"

# SETUP NODE BINARY PATH
get_node_command() {
  # First, check if NODE_BINARY is set and executable
  if [ -x "$(command -v ${NODE_BINARY})" ]; then
    echo "${NODE_BINARY}"
    return 0
  fi

  # Second, fallback to `which node`
  if [ -x "$(which node)" ]; then
    echo "node"
    return 0
  fi

  # Third, check for the highest version in the NVM directory
  NVM_NODE_VERSIONS_DIR="$HOME/.nvm/versions/node"
  if [ -d "$NVM_NODE_VERSIONS_DIR" ] && [ "$(ls -A $NVM_NODE_VERSIONS_DIR)" ]; then
    HIGHEST_VERSION=$(ls -v "$NVM_NODE_VERSIONS_DIR" | tail -n 1)
    NODE_BINARY="$NVM_NODE_VERSIONS_DIR/$HIGHEST_VERSION/bin/node"
    if [ -x "$NODE_BINARY" ]; then
      echo "$NODE_BINARY"
      return 0
    fi
  fi

  # Return empty if no node binary was found
  echo ""
  return 0
}

LOCAL_NODE_BINARY=$(get_node_command)

if [ -z "$LOCAL_NODE_BINARY" ]; then
  echo "warning: SENTRY: Node.js binary not found! Skipping symbol upload."
  exit 0
else
  echo "Using Node.js from ${LOCAL_NODE_BINARY}"
fi

# SETUP SENTRY_PROPERTIES
if [ -z "$SENTRY_PROPERTIES" ]; then
  # Check if the script is running in the root directory
  if [ -f "./sentry.properties" ]; then
    export SENTRY_PROPERTIES=sentry.properties
  elif [ -f "../../sentry.properties" ]; then
    export SENTRY_PROPERTIES=../../sentry.properties
  else
    echo "warning: SENTRY: sentry.properties file not found! Skipping symbol upload."
    exit 0
  fi
fi

echo "sentry properties found at : $(readlink -f ${SENTRY_PROPERTIES})"

# SETUP SENTRY CLI
[ -z "$SENTRY_CLI_EXECUTABLE" ] && SENTRY_CLI_PACKAGE_PATH=$("$LOCAL_NODE_BINARY" --print "require('path').dirname(require.resolve('@sentry/cli/package.json'))")
[ -z "$SENTRY_CLI_EXECUTABLE" ] && SENTRY_CLI_EXECUTABLE="${SENTRY_CLI_PACKAGE_PATH}/bin/sentry-cli"

SENTRY_COMMAND="\"$SENTRY_CLI_EXECUTABLE\" upload-dsym $SENTRY_FORCE_FOREGROUND_FLAG"

# UPLOAD DEBUG SYMBOLS
if [ "$SENTRY_SKIP_DSYM_UPLOAD" != true ]; then
  # 'warning:' triggers a warning in Xcode, 'error:' triggers an error
  set +x +e # disable printing commands otherwise we might print `error:` by accident and allow continuing on error
  SENTRY_XCODE_COMMAND_OUTPUT=$(/bin/sh -c "$LOCAL_NODE_BINARY  $SENTRY_COMMAND"  2>&1)
  if [ $? -eq 0 ]; then
    echo "$SENTRY_XCODE_COMMAND_OUTPUT"
    echo "$SENTRY_XCODE_COMMAND_OUTPUT" | awk '{print "output: sentry-cli - " $0}'
  else
    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"
    echo "error: sentry-cli - $SENTRY_XCODE_COMMAND_OUTPUT"
  fi
  set -x -e # re-enable
else
  echo "SENTRY_SKIP_DSYM_UPLOAD=true, skipping debug symbols upload"
fi

@@ -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?

Comment on lines +197 to +198
'[ -z "$SENTRY_FORCE_FOREGROUND"] && SENTRY_FORCE_FOREGROUND=true\\n' +
'[[ "$SENTRY_FORCE_FOREGROUND" == true ]] && SENTRY_FORCE_FOREGROUND_FLAG="--force-foreground"\\n' +
Copy link
Member

@krystofwoldrich krystofwoldrich Nov 4, 2024

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.

Comment on lines +199 to 219
'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' +
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

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.

2 participants