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

build: Install static lib #1948

Conversation

rzr
Copy link
Contributor

@rzr rzr commented Jul 24, 2020

This is useful for debian's iotjs-dev package

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: https://github.com/jerryscript-project/iotjs/pull/rzr
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: #1945
Bug-Debian: https://bugs.debian.org/957364
Update: 2020-07-24
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]

rzr added a commit to TizenTeam/iotjs that referenced this pull request Jul 24, 2020
This is useful for debian's iotjs-dev package

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: jerryscript-project#1948
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: jerryscript-project#1945
Bug-Debian: https://bugs.debian.org/957364
Update: 2020-07-24
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]
@rzr rzr force-pushed the sandbox/rzr/cmake/lib/review/master branch from bbadd02 to 6a23d55 Compare July 24, 2020 20:23
rzr added a commit to TizenTeam/iotjs that referenced this pull request Jul 24, 2020
This is useful for debian's iotjs-dev package

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: jerryscript-project#1948
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: jerryscript-project#1945
Bug-Debian: https://bugs.debian.org/957364
Update: 2020-07-24
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]
@@ -553,7 +556,13 @@ target_link_libraries(${TARGET_LIB_IOTJS}
${MBEDTLS_LIBS}
${EXTERNAL_LIBS}
)

target_link_libraries(${TARGET_STATIC_IOTJS}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary, the library will already be linked by the previous link_libraries call, since TARGET_LIB_IOTJS and TARGET_STATIC_IOTJS are just an alias for the same libiotjs target.

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, I just verified

@@ -497,6 +497,9 @@ set(IOTJS_PUBLIC_HEADERS

# Configure the libiotjs
set(TARGET_LIB_IOTJS libiotjs)
# Configure the libiotjs.a
set(TARGET_STATIC_IOTJS libiotjs)
Copy link
Member

Choose a reason for hiding this comment

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

I don't tnink it's necessary to add another alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not anymore

@@ -580,6 +589,7 @@ if(NOT BUILD_LIB_ONLY)
RUNTIME DESTINATION "${INSTALL_PREFIX}/bin"
LIBRARY DESTINATION "${INSTALL_PREFIX}/lib"
PUBLIC_HEADER DESTINATION "${INSTALL_PREFIX}/include/iotjs")
install(TARGETS ${TARGET_LIB_IOTJS} DESTINATION ${LIB_INSTALL_DIR})
Copy link
Member

Choose a reason for hiding this comment

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

The install command can take multiple target arguments, I'd suggest to not duplicate the calls. The install call on line 588 could be modified to be install(TARGETS ${TARGET_IOTJS} ${TARGET_LIB_IOTJS} ..., and then the calls on line 592 and 594 could be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes you're right, I am updating my patch and will update the other one that use destdir

rzr added a commit to rzr/iotjs that referenced this pull request Jul 29, 2020
This is useful for debian's iotjs-dev package

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: jerryscript-project#1948
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: jerryscript-project#1945
Bug-Debian: https://bugs.debian.org/957364
Update: 2020-07-24
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]
rzr added a commit to TizenTeam/iotjs that referenced this pull request Aug 6, 2020
This is useful for debian's iotjs-dev package

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: jerryscript-project#1948
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: jerryscript-project#1945
Bug-Debian: https://bugs.debian.org/957364
Update: 2020-07-24
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]
rzr added a commit to TizenTeam/iotjs that referenced this pull request Oct 15, 2020
Add extra install step for installing lib along headers (Fixes: jerryscript-project#1896).

Also remove headers, lib when installing executable.

This is useful for debian's iotjs-dev package

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: jerryscript-project#1948
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: jerryscript-project#1945
Bug-Debian: https://bugs.debian.org/957364
Relate-to: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Last-Update: 2020-10-15
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]
@rzr rzr force-pushed the sandbox/rzr/cmake/lib/review/master branch from 6a23d55 to 2b7a1cd Compare October 15, 2020 15:56
rzr added a commit to TizenTeam/iotjs that referenced this pull request Oct 15, 2020
Add extra install step for installing lib along headers (Fixes: jerryscript-project#1896).

Also remove headers, lib when installing executable.

This is useful for debian's iotjs-dev package

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: jerryscript-project#1948
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: jerryscript-project#1945
Bug-Debian: https://bugs.debian.org/957364
Relate-to: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Last-Update: 2020-10-15
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]
@rzr rzr force-pushed the sandbox/rzr/cmake/lib/review/master branch from 2b7a1cd to 8abaed1 Compare October 15, 2020 15:57
rzr added a commit to TizenTeam/iotjs that referenced this pull request Oct 15, 2020
installing the static lib will also install headers (Fixes: jerryscript-project#1896).

This is useful for debian's iotjs-dev package

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: jerryscript-project#1948
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: jerryscript-project#1945
Bug-Debian: https://bugs.debian.org/957364
Relate-to: jerryscript-project#1896
Last-Update: 2020-10-15
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]
@rzr rzr force-pushed the sandbox/rzr/cmake/lib/review/master branch from 8abaed1 to 1855b93 Compare October 15, 2020 16:23
rzr added a commit to TizenTeam/iotjs that referenced this pull request Oct 15, 2020
installing the static lib will also install headers (Fixes: jerryscript-project#1896).

This is useful for debian's iotjs-dev package

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: jerryscript-project#1948
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: jerryscript-project#1945
Bug-Debian: https://bugs.debian.org/957364
Relate-to: jerryscript-project#1896
Last-Update: 2020-10-15
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]
@rzr rzr force-pushed the sandbox/rzr/cmake/lib/review/master branch from 1855b93 to 2e56e9b Compare October 15, 2020 16:41
installing the static lib will also install headers (Fixes: jerryscript-project#1896).

This is useful for debian's iotjs-dev package

The extra archive rule is needed to prevent this error:

   install TARGETS given no ARCHIVE DESTINATION for static library target

Change-Id: Ib9f6cb50631f4cdfeb308108f91ed28e7d204dc4
Forwarded: jerryscript-project#1948
Origin: https://github.com/TizenTeam/iotjs/tree/sandbox/rzr/cmake/lib/review/master
Bug: jerryscript-project#1945
Bug-Debian: https://bugs.debian.org/957364
Relate-to: jerryscript-project#1896
Last-Update: 2020-10-16
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval [email protected]
@rzr rzr force-pushed the sandbox/rzr/cmake/lib/review/master branch from 2e56e9b to b13fa73 Compare October 16, 2020 09:18
@rzr
Copy link
Contributor Author

rzr commented Oct 16, 2020

review welcome

cc: @galpeter

@rzr
Copy link
Contributor Author

rzr commented Oct 21, 2020

@rzr
Copy link
Contributor Author

rzr commented Dec 9, 2020

Please review

meanwhile this change is in debian's testing branch:

https://tracker.debian.org/pkg/iotjs

[2020-10-26] iotjs 1.0+715-1 MIGRATED to testing (Debian testing watch)

cc: @akosthekiss, @dbatyai, @zherczeg

Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

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

LGTM in general but may be blocking on #1923

@rzr
Copy link
Contributor Author

rzr commented Dec 21, 2020

Thanks @akosthekiss ,

I will rebase
#1923
once
#1948
is merged

@rzr
Copy link
Contributor Author

rzr commented Apr 13, 2021

hi, can this be merged? FYI next debian release might feature this change

cc: @zherczeg @galpeter

@rzr
Copy link
Contributor Author

rzr commented May 11, 2021

Hi I plan to update iotjs in Debian,
It would help to have this change in, then I'll make a snapshot release.

On #1948 (review)
@akosthekiss said "LGTM"

I need 2 approvals to merge it, it's only a few lines to review:

b13fa73

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@rzr
Copy link
Contributor Author

rzr commented May 12, 2021

Thanks @zherczeg

Anyone for an other approval please ?

Cc: @LaszloLango , @ILyoan, @hs0225

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

LGTM

@LaszloLango LaszloLango merged commit 403f55b into jerryscript-project:master May 12, 2021
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.

5 participants