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

Include JNIEXPORT on exported symbols #300

Closed
wants to merge 1 commit into from

Conversation

carl-mastrangelo
Copy link
Member

Motivation:
As noticed in https://stackoverflow.com/questions/45700277/
compilation can fail if the definition of a method doesn't
match the declaration. It's easy enough to add this in, and make
it easy to compile.

Modification:
Add JNIEXPORT to the entry points.

  • On Windows this adds: __declspec(dllexport)
  • On Mac this adds: __attribute__((visibility("default")))
  • On Linux (GCC 4.2+) this adds: __attribute__((visibility("default")))
  • On other it doesn't add anything.

Result:
Easier compilation

@carl-mastrangelo
Copy link
Member Author

Note that this includes #299 so I don't have to rebase right after.

@normanmaurer
Copy link
Member

@carl-mastrangelo you still need to rebase this... That said LGTM

Copy link
Member

@Scottmitch Scottmitch left a comment

Choose a reason for hiding this comment

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

rebase and lgtm

Motivation:
As noticed in https://stackoverflow.com/questions/45700277/
compilation can fail if the definition of a method doesn't
match the declaration.  It's easy enough to add this in, and make
it easy to compile.

Modifications:
Add JNIEXPORT to the entry points.

* On Windows this adds: `__declspec(dllexport)`
* On Mac this adds: `__attribute__((visibility("default")))`
* On Linux (GCC 4.2+) this adds: ` __attribute__((visibility("default")))`
* On other it doesn't add anything.

Result:
Easier compilation
@carl-mastrangelo
Copy link
Member Author

@Scottmitch Done. Should I press the "Squash and Merge" button ?

@Scottmitch
Copy link
Member

I typically do it manually on the command line, which is necessary if there are multiple branches to pull the change into (GitHub only goes into the branch the PR is based off of). I think the "Squash and merge" is the same thing. We just don't want a merge commit to linger on the master branch.

git remote add carl ...
git checkout master
git cherry-pick carl/<branch>
git push origin master

@normanmaurer
Copy link
Member

@carl-mastrangelo or you could use this script:

#!/bin/bash

if [[ $# -lt 0 ]] || [[ $# -gt 2 ]]; then
  echo "Usage: $0 <pull request id> [<directory>]"
fi

PULL_REQ_ID="$1"

if [[ "x$2" != 'x' ]]; then
  cd "$2" || exit 1
fi

# Git information
GIT_STATUS="$(git status 2> /dev/null)"
if [[ "$?" != "0" ]]; then
  echo "Not a git repository: `pwd`"
  exit 2
fi

# Origin
GIT_REMOTE="$(git remote --verbose)"
if [[ ${GIT_REMOTE} =~ (origin[^a-z]*([^ ]*) *\(push\)) ]]; then
  GIT_ORIGIN="${BASH_REMATCH[2]}"
  if [[ ${GIT_ORIGIN} =~ (.*[/@]github\.com[/:]([-_a-zA-Z0-9/\.]*)\.git) ]]; then
    GITHUB_ORIGIN="${BASH_REMATCH[2]}"
  else
    echo "Not a github repository: $GIT_ORIGIN"
    exit 4
  fi
else
  echo "Not a github repository: `pwd`"
  exit 3
fi

TMPFILE=`mktemp -q -t pullreq` || echo "Failed to create a temporary file." || exit 5

echo "Fetching the pull request $PULL_REQ_ID from $GITHUB_ORIGIN .. "

PULL_REQ_URL="https://github.com/$GITHUB_ORIGIN/pull/$1.patch"
curl -sSL "$PULL_REQ_URL" > "$TMPFILE"

head -1 "$TMPFILE" | grep -q -e '^From'

if [[ $? -ne 0 ]]; then
  echo "Not a pull request: $PULL_REQ_URL"
  head -1 "$TMPFILE"
  rm -f "$TMPFILE"
  exit 6
fi

echo "Running: git am -3 $TMPFILE"
git am -3 "$TMPFILE"
rm -f "$TMPFILE"

and configure it in your .gitconfig:

        merge-pullreq = !~/scripts/git-merge-pullreq.sh

Then you can do something like this:

git merge-pullreq 300

You will then use git cherry-pick to actually pick it to other branches later on

@carl-mastrangelo
Copy link
Member Author

@normanmaurer I ran your script with some changes. Since there is only one branch for this repo, I will avoid cherry picking.

@normanmaurer
Copy link
Member

@carl-mastrangelo +1 ... Just close this issue once its merged and note the git sha here.

@carl-mastrangelo
Copy link
Member Author

Merged as 1343d60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants