Skip to content

build: Install headers to system#1649

Merged
yichoi merged 1 commit into
jerryscript-project:masterfrom
TizenTeam:sandbox/rzr/review/install/master
Jun 1, 2018
Merged

build: Install headers to system#1649
yichoi merged 1 commit into
jerryscript-project:masterfrom
TizenTeam:sandbox/rzr/review/install/master

Conversation

@rzr
Copy link
Copy Markdown
Contributor

@rzr rzr commented May 22, 2018

make install will install headers to iotjs headers namespace.
This is needed for build developer packages,
on debian based system like raspbian for pi0.

This change was applied to debian packaging for supporting RPI.

Bug: #1145
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval philippe.coval@osg.samsung.com

Comment thread cmake/iotjs.cmake Outdated
endif()

# Install headers
if("${INCLUDE_INSTALL_DIR}" STREQUAL "")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems better to put the relevant codes in one place. I suggest moving this code block to the line, 488.

Comment thread cmake/iotjs.cmake Outdated
if("${INCLUDE_INSTALL_DIR}" STREQUAL "")
set(INCLUDE_INSTALL_DIR "include/iotjs")
endif()
install(TARGETS ${TARGET_LIB_IOTJS} DESTINATION ${LIB_INSTALL_DIR})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isn't TARGET_LIB_IOTJS undefined?

Comment thread cmake/iotjs.cmake Outdated
set(INCLUDE_INSTALL_DIR "include/iotjs")
endif()
install(TARGETS ${TARGET_LIB_IOTJS} DESTINATION ${LIB_INSTALL_DIR})
file(GLOB IOTJS_HEADERS include/*.js src/*.h)
Copy link
Copy Markdown
Member

@daeyeon daeyeon May 24, 2018

Choose a reason for hiding this comment

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

a) include/.js -> include/.h?
b) can you share use cases for headers of src/*.h?

@rzr rzr force-pushed the sandbox/rzr/review/install/master branch 2 times, most recently from 73c9598 to 54d3c02 Compare May 24, 2018 17:07
Comment thread cmake/iotjs.cmake Outdated
${MBEDTLS_LIBS}
-Wl,--no-whole-archive
${EXTERNAL_LIBS})
install(TARGETS ${TARGET_SHARED_IOTJS} DESTINATION ${LIB_INSTALL_DIR})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

wrong indentation

make install will install headers to iotjs headers namespace.
This is needed for build developer packages,
on debian based system like raspbian for pi0.

This change was applied to 1.0 for debian packaging for supporting RPI,
and then improved to align to master.

Bug: jerryscript-project#1145
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval philippe.coval@osg.samsung.com
@rzr rzr force-pushed the sandbox/rzr/review/install/master branch from 54d3c02 to cda7b5d Compare May 31, 2018 09:31
Copy link
Copy Markdown
Contributor

@hs0225 hs0225 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@yichoi yichoi left a comment

Choose a reason for hiding this comment

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

LGTM

@yichoi yichoi merged commit 3d191b1 into jerryscript-project:master Jun 1, 2018
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.

4 participants