Skip to content

rocks: unable to install with tarantoolctl rocks on Linux #44

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 25, 2022
Merged

rocks: unable to install with tarantoolctl rocks on Linux #44

merged 1 commit into from
Jan 25, 2022

Conversation

vr009
Copy link
Contributor

@vr009 vr009 commented Dec 4, 2021

The problem from #39 is solved by adding EXCLUDE_FROM_ALL option.

On any platforms many targets from subdirectories are included into make command by default. They are not necessary to build driver.{so|dylib}. EXCLUDE_FROM_ALL option disables all targets at the beginning and only targets which are explicitly added to mqtt dependencies will be built.

Additionally static build for rocks set by default.

Fixes #39

@vr009 vr009 requested review from Totktonada and ligurio December 4, 2021 00:21
ligurio
ligurio reviewed Dec 7, 2021
View reviewed changes
Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!
Please limit a width of commit message by 72 symbols, see [1].

  1. https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/#how-to-write-a-commit-message

@ligurio
Copy link
Member

ligurio commented Dec 7, 2021

The problem from #39 is solved by adding EXCLUDE_FROM_ALL option.

I see that issue #39 collects a number of problems:

  1. installation of /usr/local/etc/mosquitto requires administrative privileges
  2. error on linking executable mosquitto_rr

Seems your commit solves only the first problem. What about second one?

@ligurio
Copy link
Member

ligurio commented Dec 7, 2021

Additionally static build for rocks set by default.

What for?

@vr009 vr009 force-pushed the vr009/gh-39-fix-installation-with-rocks branch from 27f29a0 to c8a61f2 Compare December 9, 2021 03:18
@vr009
Copy link
Contributor Author

vr009 commented Dec 9, 2021

I see that issue #39 collects a number of problems:

  1. installation of /usr/local/etc/mosquitto requires administrative privileges
  2. error on linking executable mosquitto_rr

Seems your commit solves only the first problem. What about second one?

  1. The requirement of administrative privileges was caused by some internal targets from libmosquitto subdirectories.
    They are not necessary to build module's driver. EXCLUDE_FROM_ALL option disables all the targets in current subdirectory by default. So those targets, which are explicitly added to mqtt dependencies, will be built.
  2. Error on linking executable mosquitto_rr was solved here: rr_client redefines 'cfg' eclipse-mosquitto/mosquitto#1453.
    We should update mosquitto to at least 1.6.8.

@vr009
Copy link
Contributor Author

vr009 commented Dec 9, 2021

Additionally static build for rocks set by default.

What for?

The idea was, that installation from rocks is working everywhere without need to install any library manually. That is default behavior. But it is still possible to build the module with additional options for CMake, if it is needed.

@ligurio
Copy link
Member

ligurio commented Dec 9, 2021

Additionally static build for rocks set by default.

What for?

The idea was, that installation from rocks is working everywhere without need to install any library manually. That is default behavior. But it is still possible to build the module with additional options for CMake, if it is needed.

Let's add explanation to a commit message.

@vr009 vr009 force-pushed the vr009/gh-39-fix-installation-with-rocks branch from c8a61f2 to 01694e6 Compare December 9, 2021 14:02
@vr009 vr009 requested a review from ligurio December 14, 2021 11:18
@vr009
Copy link
Contributor Author

vr009 commented Dec 15, 2021

Additionally static build for rocks set by default.

What for?

The idea was, that installation from rocks is working everywhere without need to install any library manually. That is default behavior. But it is still possible to build the module with additional options for CMake, if it is needed.

Let's add explanation to a commit message.

Done.

ligurio
ligurio suggested changes Dec 28, 2021
View reviewed changes
@vr009 vr009 force-pushed the vr009/gh-39-fix-installation-with-rocks branch from 01694e6 to 2356a58 Compare January 6, 2022 09:28
@vr009 vr009 requested a review from ligurio January 6, 2022 09:31
@vr009 vr009 force-pushed the vr009/gh-39-fix-installation-with-rocks branch from 2356a58 to e4c9813 Compare January 12, 2022 15:11
@vr009
rocks: unable to install with tarantoolctl rocks on Linux
a18c852 
On any platforms many targets from subdirectories are included into make
command by default. They are not necessary to build driver.{so|dylib}.
EXCLUDE_FROM_ALL option disables all targets at the beginning and only targets
which are explicitly added to mqtt dependencies will be built.

Static build option for libmosquitto set by default in rocks. That means there
is no more need to install libmosquitto manually for successful build from rocks.
But there is still an opportunity to add some other build options in CMake,
if it is needed.

WITH_PIC added as an option. This is related to the CMP0077 policy.
https://cmake.org/cmake/help/latest/policy/CMP0077.html. Without this
change the behaviour is the following. If the module builds statically,
WITH_PIC is firstly set on, but the effect of this will be discarded by
option(WITH_PIC <...> OFF) in third_party/mosquitto/lib/CMakeLists.txt.
This way the build fails.

Fixes #39
@vr009 vr009 force-pushed the vr009/gh-39-fix-installation-with-rocks branch from e4c9813 to a18c852 Compare January 12, 2022 17:26
Totktonada
Totktonada approved these changes Jan 12, 2022
View reviewed changes
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

LGTM.

ligurio
ligurio approved these changes Jan 25, 2022
View reviewed changes
Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

LGTM

@Totktonada Totktonada merged commit db28374 into master Jan 25, 2022
@Totktonada Totktonada deleted the vr009/gh-39-fix-installation-with-rocks branch January 25, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Totktonada Totktonada Totktonada approved these changes

@ligurio ligurio ligurio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to install with tarantoolctl rocks on Linux
3 participants
@vr009 @ligurio @Totktonada