feature: exposing headers for libmirsever-internal-dev

[mattkae]

What's new?

Exposing a new package called libmirserver-internal-dev

How to use

pkg_check_modules(MIRSERVER_INTERNAL mirserver-internal REQUIRED)
...
target_include_directories(${BINARY_NAME} PUBLIC SYSTEM ... ${MIRSERVER_INTERNAL_INCLUDE_DIRS})

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.35%. Comparing base (3293097) to head (1652ceb).
Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3288      +/-   ##
==========================================
+ Coverage   77.33%   77.35%   +0.01%     
==========================================
  Files        1062     1075      +13     
  Lines       67817    68268     +451     
==========================================
+ Hits        52449    52806     +357     
- Misses      15368    15462      +94     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AlanGriffiths]

I'm wondering if it would be cleaner to use include/server-internal for these headers instead of src/include/server. That keeps all the published stuff in the include tree.

@RAOF any thoughts?

[mattkae]

I'm wondering if it would be cleaner to use include/server-internal for these headers instead of src/include/server. That keeps all the published stuff in the include tree.

@RAOF any thoughts?

I personally like having them exposed from src/include/... as it makes it obvious that they are internal to the project compared to the include/... headers.

[AlanGriffiths]

I'd like @RAOF's eyes on too. But ok by me

[Conan-Kudo]

Why do we want this? Can we do something to make it so that if we're using these internal methods, the linkage indicates it somehow similar to how Qt does it?

[mattkae]

Why do we want this? Can we do something to make it so that if we're using these internal methods, the linkage indicates it somehow similar to how Qt does it?

I am unfamiliar with how Qt does it. Do they somehow provide you with a big warning that "you're doing something funky"?

[mattkae]

Why do we want this?

Also, we want this so that you can gain access to "unforeseen" parts of Mir (e.g. I want access to the transformation on scene::Surfaces and the renderer later on). Ideally, these things would be provided through miral IMO, but this is a bandaid until those interfaces are created

[RAOF]

I'm happy to keep these headers in src/include.

A question, though: how many of these headers are usable outside of a Mir build? We don't guarantee any of the symbols here get exported, so you might be able to write code that compiles but does not link? Have you tested using these headers?

[Conan-Kudo]

Why do we want this? Can we do something to make it so that if we're using these internal methods, the linkage indicates it somehow similar to how Qt does it?

I am unfamiliar with how Qt does it. Do they somehow provide you with a big warning that "you're doing something funky"?

Qt has a concept of "private APIs" that are explicitly declared as unstable and when something uses them, they get a link-time dependency symbol version of Qt_6.6_PRIVATE_API.

You can see an example of this in plasma-workspace-libs in Fedora 40: https://koji.fedoraproject.org/koji/rpminfo?rpmID=38028890

This is done with a version script generated by their CMake: https://github.com/qt/qtbase/blob/dev/cmake/QtFlagHandlingHelpers.cmake#L72

Fedora and openSUSE patch this to use the full Qt major.minor: https://src.fedoraproject.org/rpms/qt6-qtbase/blob/rawhide/f/qtbase-tell-the-truth-about-private-API.patch

[mattkae]

I'm happy to keep these headers in src/include.

👍

A question, though: how many of these headers are usable outside of a Mir build? We don't guarantee any of the symbols here get exported, so you might be able to write code that compiles but does not link? Have you tested using these headers?

I've tested using some, but not all. However, wouldn't mirserver (that gets built by src/server/CMakeLists.txt) contain the built library and everything that exports from it?

[RAOF]

A question, though: how many of these headers are usable outside of a Mir build? We don't guarantee any of the symbols here get exported, so you might be able to write code that compiles but does not link? Have you tested using these headers?

I've tested using some, but not all. However, wouldn't mirserver (that gets built by src/server/CMakeLists.txt) contain the built library and everything that exports from it?

libmirserver.so will definitely contain the object code for everything in the headers (because we use it internally), but src/server/symbols.map has a local: * stanza at the end, which means that any symbols we don't explicitly export above will not be generated (so linking will fail).

Since the stuff in src/include is not expected to be used outside, I don't think we'll have guaranteed that the relevant symbols are exported?

[mattkae]

Since the stuff in src/include is not expected to be used outside, I don't think we'll have guaranteed that the relevant symbols are exported?

Oof ok that makes sense to me.

I am assuming that this means that we must export those symbols in Mir in another library potentially (e.g. libmirserver-internal.so). This library must be the exact opposite of src/server/symbols.map in that only files that are local should be global.

Do you agree @RAOF?

[RAOF]

To capture our private conversation: no, these symbols should be exposed from libmirserver.so along with the rest of our normal symbols, but instead they should be exposed in a internal_MIR_$MAJOR.$MINOR.$PATCH stanza.

This would also be a really good time to work out how to generate our symbols list, so we can automatically mark everything in the internal headers as internal_* and everything else with the version we expect.

Needs re-review

[Conan-Kudo]

To capture our private conversation: no, these symbols should be exposed from libmirserver.so along with the rest of our normal symbols, but instead they should be exposed in a internal_MIR_$MAJOR.$MINOR.$PATCH stanza.

This would also be a really good time to work out how to generate our symbols list, so we can automatically mark everything in the internal headers as internal_* and everything else with the version we expect.

Do we really need it to include patch versions too? I feel that's unnecessarily painful.

[mattkae]

@RAOF : Do I need a debian/libmirserver-internal-dev.symbols file as well?

[RAOF]

@RAOF : Do I need a debian/libmirserver-internal-dev.symbols file as well?

No. The debian/$PACKAGE.symbols file for the package that contains the shared library; the shared library in this case is in the libmirserver$SOVER package.

Do we really need it to include patch versions too? I feel that's unnecessarily painful.

I don't think it'll be painful for us :)

It depends on what ABI stability we want to promise, of course. I don't think that promising not to break internal symbols in a patch release will be arduous for us, so, yeah, it should probably be MIR_SERVER_INTERNAL_$MAJOR.$MINOR.

[RAOF]

These look like they're meant to be the internal symbols?

[AlanGriffiths]

These are also "INTERNAL" symbols - and should also be in the MIR_SERVER_INTERNAL_2.17 stanza

[AlanGriffiths]

It depends on what ABI stability we want to promise, of course. I don't think that promising not to break internal symbols in a patch release will be arduous for us, so, yeah, it should probably be MIR_SERVER_INTERNAL_$MAJOR.$MINOR.

I agree. But note it isn't just symbols about, we need to track general ABI breakages regardless (for the .soname).

There is also the option of an additional MIR_SERVER_INTERNAL_$MAJOR.$MINOR.$PATCH if a patch introduces symbols in an ABI compatible way.

[AlanGriffiths]

The following are the "INTERNAL" symbols

[AlanGriffiths]

These are also "INTERNAL" symbols - and should also be in the MIR_SERVER_INTERNAL_2.17 stanza

[AlanGriffiths]

Looks sensible (apart from .soname needing a bump)

But we will likely wait on parent landing before landing this

[mattkae]

It isn't immediately obvious why it has started here, but the clang build is failing with

usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23

Are we provoking an argument between clang and ld? (I habitually use ldd with clang - because ld used to complain at one point)

I think that I just fixed this using mold. If it's still failing, I'll try ldd too

[AlanGriffiths]

Looks reasonable. One naming nit

[AlanGriffiths]

Suggested change

	def derived_virtual_base_class(node: clang.cindex.Cursor):
	def has_virtual_base_class(node: clang.cindex.Cursor):

[mattkae]

Done 👍