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: module compilation within termux-app on android #1340

Merged
merged 1 commit into from
Oct 20, 2018
Merged

Fix: module compilation within termux-app on android #1340

merged 1 commit into from
Oct 20, 2018

Conversation

arrkiin
Copy link
Contributor

@arrkiin arrkiin commented Nov 16, 2017

Description of change

To solve the problem of linking a compiled module against a preinstalled library of termux-app on android I added a special condition branch to add the required -fPIC flag for the c and c++ compiler. After that everything compiled, linked and works as expected.

addon.gypi Outdated
[ 'OS == "android"', {
'cflags': [ '-fPIC' ],
'cflags_cc': ['-fPIC' ],
}],
Copy link
Member

Choose a reason for hiding this comment

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

You can fold this into the previous condition: OS in "android freebsd ...". You don't need to add it to cflags_cc because cflags is for both C and C++ code.

Copy link
Contributor Author

@arrkiin arrkiin Nov 16, 2017

Choose a reason for hiding this comment

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

@bnoordhuis thanks for your comment. Your proposal was my first attemp to fix it, but it didn't. To verify if I didn't make any mistake I just did some testing:

  • with 'cflags_cc': ['-fPIC' ] it compiles
  • without 'cflags_cc': ['-fPIC' ] it does not

I think I have to compare the verbose outputs in more detail to figure out what happens. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

env V=1 node-gyp rebuild turns on verbose compilation mode (i.e., prints the compile commands.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the compiler command for the variant without ccflags_cc. It seems that the -fPIE after the first -fPIC creates and unlinkable shared library on android.
... -fPIC -Wall -Wextra -Wno-unused-parameter -O3 -fno-omit-frame-pointer -fPIE -fno-rtti ...

With ccflags_cc the command is build like that. It seems, that the later -fPIC overrides the preceding -fPIE so that it produces the right shared library.
... -fPIC -Wall -Wextra -Wno-unused-parameter -O3 -fno-omit-frame-pointer -fPIE -fPIC -fno-rtti ...

With this result I would rather try to remove the -fPIE while compiling shared libraries on android. I added an additional entry in target_condition for removing -fPIE like shown below. That did the trick and produces the following compiler command.
... -fPIC -Wall -Wextra -Wno-unused-parameter -O3 -fno-omit-frame-pointer -fno-rtti ...

Excerpt of my local addon.gypi:

'target_conditions': [
...
      ['library=="shared_library"', {
        'conditions': [
          [ 'OS=="android"', {
            'cflags!': [ '-fPIE' ],
          }]
        ]
      }],
...
]

@bnoordhuis, what is your comment on this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're probably getting hit by #1118 - that -fPIE flag is not set by node-gyp, it's inherited from node's common.gypi.

The right thing to do until #1118 is resolved is probably to suppress the flag in node-gyp's addon.gypi.

It must have been broken for quite a while because that flag was added 20 months ago in nodejs/node#5544.

addon.gypi Outdated
@@ -52,6 +52,14 @@
'standalone_static_library': '<(standalone_static_library)'
}],

['library=="shared_library"', {
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to unset -fPIE unconditionally, otherwise it's still going to be problematic with add-ons that also build static libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. So we only want to remove the flag if we don't want to compile an executable. Am I right?

@bnoordhuis
Copy link
Member

LGTM, thanks. CI: https://ci.nodejs.org/job/nodegyp-test-pull-request/42/

You don't even need to special-case on OS=="android" when removing -fPIE but this is good enough.

@arrkiin
Copy link
Contributor Author

arrkiin commented Nov 20, 2017

@bnoordhuis, when can I expect it to be merged and published?

@bnoordhuis
Copy link
Member

We don't release that often and it also takes a while for new versions of node-gyp make it into npm and node.

I can merge it but can you squash your commits and write up a nice commit log? If you want to show up under your real name instead of arrkiin, please do git commit --amend --author="...". Cheers.

@pietvanzoen
Copy link

Hey folks. Would love to see this merged. Running into this issue currently. :)

@arrkiin
Copy link
Contributor Author

arrkiin commented May 22, 2018

Hey @bnoordhuis, sorry for the long delay. After your message I asked myself how to proceed and forgot to continue. In the meantime I used my own fork to do the experiments under termux. Until now I didn't see any problems. Today I just found some time to squash the commits and add my forename.

@maclover7
Copy link
Contributor

@johnnyvf24
Copy link

johnnyvf24 commented Aug 14, 2018

Hey folks. Would love to see this merged. Running into this issue currently. :)

I can second that.

@refack
Copy link
Contributor

refack commented Oct 20, 2018

@refack refack self-assigned this Oct 20, 2018
@refack refack merged commit e2401e1 into nodejs:master Oct 20, 2018
rvagg pushed a commit that referenced this pull request Apr 24, 2019
@rvagg rvagg mentioned this pull request Apr 24, 2019
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.

6 participants