Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

Conversation

gabrielschulhof
Copy link

If you configure soletta to not have OIC functionality then the resulting node.js bindings can't have such functionality either.

Addtitionally, this PR fixes #1567 by surrounding the addition of the fortify-related CFLAGS with a check to make sure that -O0 is not already part of the CFLAGS.

@gabrielschulhof gabrielschulhof changed the title Nodejs bindings follow config settings WIP: Nodejs bindings follow config settings Mar 30, 2016
Gabriel Schulhof added 2 commits March 30, 2016 23:18
This sacrifices our ability to build bindings against an existing soletta
installation identified by its pkgconfig file, because the pkgconfig file does
not contain information about the configuration options that were used when
building the package.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes solettaprojectgh-1567

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@gabrielschulhof gabrielschulhof force-pushed the nodejs-bindings-follow-config-settings branch from dff591d to 8a3c3d7 Compare March 30, 2016 20:18
@gabrielschulhof gabrielschulhof changed the title WIP: Nodejs bindings follow config settings Nodejs bindings follow config settings Mar 30, 2016
COMMON_CFLAGS += $(RELEASE_COMMON_CFLAGS)
ifeq (,$(shell echo $(COMMON_CFLAGS) | grep -e "-O0"))
COMMON_CFLAGS += $(FORTIFY_CFLAGS)
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to have a release build with -O0 ?

Copy link
Author

Choose a reason for hiding this comment

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

Well, I've tried building with

BUILD_TYPE_RELEASE=n make

and it seems to ignore that, whereas building with

COMMON_CFLAGS="-g -O0" make

correctly prepends -g -O0 to the COMMON_CFLAGS.

The bottom line is that COMMON_CFLAGS may be set from outside the build environment, so the build environment has to account for this. Now, most of the time the COMMON_CFLAGS arriving from the outside are orthogonal to what we're adding inside the build environment, but in this case the existing COMMON_CFLAGS need to take precedence.

Copy link
Author

Choose a reason for hiding this comment

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

Additional info: The use case for this is the situation where the user starts the build process with
npm install or npm install --debug. In both cases the npm install process ends up calling make alldefconfig followed by make, but prefixed with a bunch of configuration options. In case of npm install, it results in

USE_NODEJS="y" RPATH="y" make

whereas in the npm install --debug case it also has to at least turn on -g -O0 in Soletta. It does this by also adding COMMON_CFLAGS="-g -O0" to the settings preceding the make.

I also don't necessarily want to link Soletta's debug build feature to node.js' debug build, considering -g -O0 as sufficient debugging support by Soletta for the node.js bindings.

Here's how it is currently:

make alldefconfig || exit 1
if test "x${npm_config_debug}x" = "xtruex"; then
sed -i .config -r -e 's/(# )?CONFIG_CFLAGS.*$/CONFIG_CFLAGS="-g -O0"/'
fi
USE_NODEJS="y" RPATH="y" make || exit 1

There are two problems with how it is currently:

  1. It modifies the .config file. This is fixed in this PR by conditionally setting COMMON_CFLAGS for make instead.
  2. -O0 from external COMMON_CFLAGS is being overridden by RELEASE_COMMON_CFLAGS which later adds -O2 so as to support -DFORTIFY_SOURCE=2. This currently cannot be avoided by either modifying .config or by prefixing make with a variable AFAICT.

Copy link
Author

Choose a reason for hiding this comment

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

OK, so I understand why I can't use BUILD_TYPE_RELEASE=n make. It's because Makefile includes .config, and in .config BUILD_TYPE_RELEASE is set explicitly to y, not via ?=, so the environment variable is being ignored. Thus, my only option, if I wish not to modify the .config file is to use COMMON_CFLAGS which, in turn, leaves me with the solution in this PR.

@bdilly
Copy link
Contributor

bdilly commented Mar 31, 2016

What I'm suggesting is to use BUILD_TYPE_DEBUG=y instead of BUILD_TYPE_RELEASE=y (on this case BUILD_TYPE_RELEASE isn't even declared)

@gabrielschulhof gabrielschulhof deleted the nodejs-bindings-follow-config-settings branch April 8, 2016 08:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-O present in CFLAGS should have precedence over _FORTIFY_SOURCE
2 participants