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

Give better names to shared libraries #1564

Merged
merged 1 commit into from
Feb 19, 2020
Merged

Give better names to shared libraries #1564

merged 1 commit into from
Feb 19, 2020

Conversation

lbonn
Copy link
Contributor

@lbonn lbonn commented Feb 18, 2020

Use set_target_properties so that we don't confuse the name of library
and executable targets.

@lbonn
Copy link
Contributor Author

lbonn commented Feb 18, 2020

Note: it will require an update on meta-updater

pattivacek
pattivacek previously approved these changes Feb 18, 2020
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.

Does this change the linking at all, or is cmake smart enough to sort it out automatically?

@mike-sul
Copy link
Collaborator

It requires adjusting of this https://github.com/advancedtelematic/homebrew-otaconnect/blob/a4b13aec79ff2dd8896973a935155185d226eb66/aktualizr.rb#L34

@lbonn
Copy link
Contributor Author

lbonn commented Feb 18, 2020

Does this change the linking at all, or is cmake smart enough to sort it out automatically?

What do you mean by "change the linking"?

We now have:

$ ninja -v aktualizr
...
[2/2] : && /usr/bin/c++ ...  src/libaktualizr/libaktualizr.so ...

So:

$ ldd src/aktualizr_primary/aktualizr
        linux-vdso.so.1 (0x00007fff5a10c000)
        libaktualizr.so => .../build/src/libaktualizr/libaktualizr.so (0x00007f7fbb5aa000)

Use set_target_properties so that we don't confuse the name of library
and executable targets.

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
@lbonn
Copy link
Contributor Author

lbonn commented Feb 18, 2020

@mike-sul

It requires adjusting of this advancedtelematic/homebrew-otaconnect:aktualizr.rb@a4b13ae#L34

Can we use make install in these steps or does it bring some complications?

@pattivacek
Copy link
Collaborator

What do you mean by "change the linking"?

You actually answered exactly what I was looking for in your comment ("Note: it will require an update on meta-updater") but I didn't see it somehow. :)

I assume https://github.com/advancedtelematic/libaktualizr-demo-app will also need an update.

@codecov-io
Copy link

codecov-io commented Feb 18, 2020

Codecov Report

Merging #1564 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1564      +/-   ##
=========================================
- Coverage   82.14%   82.1%   -0.05%     
=========================================
  Files         189     189              
  Lines       11840   11840              
=========================================
- Hits         9726    9721       -5     
- Misses       2114    2119       +5
Impacted Files Coverage Δ
src/libaktualizr/storage/sql_utils.h 84.72% <0%> (-1.39%) ⬇️
src/libaktualizr/storage/sqlstorage.cc 77.39% <0%> (-0.68%) ⬇️
src/libaktualizr/primary/sotauptaneclient.cc 90% <0%> (-0.38%) ⬇️
src/libaktualizr/package_manager/ostreemanager.cc 79.04% <0%> (+0.36%) ⬆️
src/libaktualizr/storage/sqlstorage_base.cc 79.72% <0%> (+3.37%) ⬆️

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 b0e83f4...35fe41a. Read the comment docs.

@mike-sul
Copy link
Collaborator

mike-sul commented Feb 19, 2020

@mike-sul

It requires adjusting of this advancedtelematic/homebrew-otaconnect:aktualizr.rb@a4b13ae#L34

Can we use make install in these steps or does it bring some complications?

Yes, agree, I think it will be the correct way to do it in the formula and I'll try to do it. In any case feel free to merge this change since it won't impact the formula anyway since it builds & installs 2020.2 version.

@lbonn
Copy link
Contributor Author

lbonn commented Feb 19, 2020

Ok great, then I think it's close to be ready. Just one question remains IMO is whether we should also care about versions in .so names, like what is done on a typical distribution (ie: libaktualizr.so.0, libaktualizr.so.2019.2...).

As a system typically contains only one application (or two including aktualizr-info) linking with these .so, I would say it's probably fine in the current state?

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.

Just one question remains IMO is whether we should also care about versions in .so names, like what is done on a typical distribution (ie: libaktualizr.so.0, libaktualizr.so.2019.2...).

As a system typically contains only one application (or two including aktualizr-info) linking with these .so, I would say it's probably fine in the current state?

Interesting idea. I think I agree that for now it shouldn't be necessary, though.

@lbonn lbonn merged commit be85b3a into master Feb 19, 2020
@lbonn lbonn deleted the fix/dynlib-name branch February 19, 2020 10:39
@mike-sul
Copy link
Collaborator

Ok great, then I think it's close to be ready. Just one question remains IMO is whether we should also care about versions in .so names, like what is done on a typical distribution (ie: libaktualizr.so.0, libaktualizr.so.2019.2...).

As a system typically contains only one application (or two including aktualizr-info) linking with these .so, I would say it's probably fine in the current state?

Effectively, customers do not have a mean to update just aktualizr/libaktualizr, they are updated via updating an overall ostree-based rootfs, so the packaging unit is at an ostree revision which means that this library versioning does not bring much value. On a flip side, versioning of libaktualizr could clearly designate what version it is actually is.

BTW, perhaps it makes sense to consider updating of aktualizr without a need in bitbaking an overall rootfs for each device, so customers will have much simpler way to update it.

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.

None yet

4 participants