Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Build with shared boost libraries #1067

Merged
merged 2 commits into from
Jan 24, 2019
Merged

Build with shared boost libraries #1067

merged 2 commits into from
Jan 24, 2019

Conversation

eu-siemann
Copy link
Contributor

I'm not sure if we should keep linking with static libraries for the debian case, or make it dynamic everywhere.
This might also break meta-updater due to changed run-time dependencies.
Otherwise, I see no benefits in linking boost statically.

@eu-siemann
Copy link
Contributor Author

@patriotyk Do you remember, why you've changed it to static?

Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

As far as I can tell from the history, it was done out of necessity for the Debian build. It probably did not need to be applied everywhere.

That said, we should test this in meta-updater before merging just to be sure there aren't any unexpected consequences.

@@ -42,8 +42,8 @@ configure_file(CTestCustom.cmake CTestCustom.cmake)

unset(AKTUALIZR_CHECKED_SRCS CACHE)

set(CMAKE_POSITION_INDEPENDENT_CODE ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

That isn't the default?

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 property is True by default for SHARED and MODULE library targets and False otherwise.

@lbonn
Copy link
Contributor

lbonn commented Jan 23, 2019

I would like to avoid compiling differently depending on BUILD_DEB: we've tried to keep these build options independent from each others, only adding functionality.
It allows us to run the full test suite with everything (almost?) to =on while being confident that it's not too far off from the real system. (maybe counter-examples still linger around though..?).

If we really need special compilation options for a particular target, it could be done through CMAKE_BUILD_TYPE or another cmake switch.

@eu-siemann
Copy link
Contributor Author

@lbonn Good point, I agree that this compilation option shouldn't change how the whole project is built.
The question is, do we really need static boost libraries for the debian package? And If yes, why we don't need e.g. static libsoduim?

@pattivacek
Copy link
Collaborator

do we really need static boost libraries for the debian package?

Probably not. Let's see how far we can get without it.

@eu-siemann eu-siemann force-pushed the feat/dyn-boost branch 2 times, most recently from c5adb40 to c4c66d0 Compare January 23, 2019 10:22
@codecov-io
Copy link

codecov-io commented Jan 23, 2019

Codecov Report

Merging #1067 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1067      +/-   ##
==========================================
+ Coverage   75.21%   75.23%   +0.02%     
==========================================
  Files         154      154              
  Lines        8901     8901              
==========================================
+ Hits         6695     6697       +2     
+ Misses       2206     2204       -2
Impacted Files Coverage Δ
src/libaktualizr/package_manager/ostreemanager.cc 55.7% <0%> (+0.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c49e6e...e7d4546. Read the comment docs.

@eu-siemann eu-siemann changed the title Build with static boost libs for debian package only Build with shared boost libraries Jan 23, 2019
@eu-siemann
Copy link
Contributor Author

@patrickvacek I've built a rocko qemu image with this aktualizr version and it works, the device was provisioned successfully.

libboost-regex1.65.1 \
libboost-system1.65.1 \
libboost-test1.65.1 \
libboost-thread1.65.1 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why random and regex? I thought we didn't use those anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patrickvacek I simply took it from the Dockerfile.ubuntu.bionic. If we don't use it, then it should be also deleted there, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still use random in Utils.Base64RoundTrip test, but looks like regex could be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The is no point in using boost::random over std::random there, so I'll try to remove it as well.

Eugene Smirnov added 2 commits January 23, 2019 14:53
Signed-off-by: Eugene Smirnov <evgenii.smirnov@here.com>
Signed-off-by: Eugene Smirnov <evgenii.smirnov@here.com>
Copy link
Contributor

@OYTIS OYTIS left a comment

Choose a reason for hiding this comment

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

I'm happy to get rid of statically linking boost too.

By the way, do we still have users on debian? Our debian support seems quite rudimental TBH.

@pattivacek
Copy link
Collaborator

By the way, do we still have users on debian?

Not to my knowledge. It's mostly just useful for testing and demonstration these days. We could consider getting rid of it if the maintenance burden is too high, but it hasn't seemed too bad so far.

To be clear, the debian releases of garage-deploy are in use, it's just the debian package manager in libaktualizr that is unused.

@pattivacek pattivacek merged commit e4b246a into master Jan 24, 2019
@pattivacek pattivacek deleted the feat/dyn-boost branch January 24, 2019 13:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants