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

Bumping mircommon, miral, and mirserver sonames for mircookie work #3178

Merged

Conversation

mattkae
Copy link
Contributor

@mattkae mattkae commented Jan 9, 2024

No description provided.

@mattkae mattkae requested a review from AlanGriffiths January 9, 2024 16:36
@mattkae mattkae marked this pull request as ready for review January 9, 2024 16:36
Copy link
Collaborator

@AlanGriffiths AlanGriffiths 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 trying, but you're not done yet!

For the libs you've touched, there need to be changes to libmiral6.symbols

And there are more libs to touch: I think that mircore is the only ABI that isn't being broken.

For example, miroil depends on mircommon. So a process needs to be able to load a consistent versions of both libraries. That is only possible with a bump to the miroil .soname.

The same applies to, for example, platforms

@@ -81,7 +81,7 @@ def _report(publish, symbol):
else:
print('NOPUBLISH: {}'.format(symbol))

OLD_STANZAS = '''MIRAL_4.0 {
OLD_STANZAS = '''MIRAL_4.1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be going to MIRAL_5.0

global:'''

END_NEW_STANZA = '''
} MIRAL_4.0;
} MIRAL_4.1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to clear these symbols from the script and let it fill in the current stanza. See 8e3130d for an example

MIR_SERVER_2.15 {
MIR_SERVER_2.16 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MIR_SERVER_2.17 - 2.16 has already been released

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! Why did it say 2.15? Because we didn't introduce any new symbols on the prev release?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly - the stanza is the release the symbol was introduce to the current .soname

@@ -1,4 +1,4 @@
MIR_COMMON_2.8 {
MIR_COMMON_2.15 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MIR_SERVER_2.17 - 2.15 has already been released

Comment on lines 4 to 5
set(MIRAL_VERSION_MAJOR 4)
set(MIRAL_VERSION_MINOR 1)
set(MIRAL_VERSION_MINOR 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

5.0, not 4.2

@mattkae
Copy link
Contributor Author

mattkae commented Jan 9, 2024

For the libs you've touched, there need to be changes to libmiral6.symbols

Do I do this part by hand?

For example, miroil depends on mircommon. So a process needs to be able to load a consistent versions of both libraries. That is only possible with a bump to the miroil .soname.

The same applies to, for example, platforms

Kk makes sense!

@mattkae
Copy link
Contributor Author

mattkae commented Jan 9, 2024

@AlanGriffiths does that include mirwayland?

@AlanGriffiths
Copy link
Collaborator

@AlanGriffiths does that include mirwayland?

target_link_libraries(mirwayland
  PUBLIC
    mircore
    PkgConfig::WAYLAND_SERVER
  PRIVATE
    mircommon
)

@AlanGriffiths
Copy link
Collaborator

For the libs you've touched, there need to be changes to libmiral6.symbols

Do I do this part by hand?

The check-and-update-debian-symbols.py (runs as part of a build) adds missing symbols. So just strip all the symbols and cmake -B ....

@mattkae mattkae force-pushed the feature/remove_mir_cookie_sonames branch from 5e7bf68 to 4bc086e Compare January 10, 2024 16:52
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (63f79d9) 77.80% compared to head (9f79f3b) 77.81%.

❗ Current head 9f79f3b differs from pull request most recent head f58f79c. Consider uploading reports for the commit f58f79c to get more accurate results

Additional details and impacted files
@@                    Coverage Diff                     @@
##           feature/remove_mir_cookie    #3178   +/-   ##
==========================================================
  Coverage                      77.80%   77.81%           
==========================================================
  Files                           1062     1062           
  Lines                          74626    74650   +24     
==========================================================
+ Hits                           58063    58087   +24     
  Misses                         16563    16563           

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

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

We also need to bump the platform ABIs

  • MIR_SERVER_INPUT_PLATFORM_ABI
  • MIR_SERVER_GRAPHICS_PLATFORM_ABI

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a regenerate-miroil-symbols-map.py (and build target) too

@RAOF
Copy link
Contributor

RAOF commented Jan 12, 2024

I am confused by the failures here. It looks like miroil symbols used to be just ignored (libmiroil3.symbols when the library was libmiroil.so.4), and the miroil symbols diff is removing and then adding back the same symbols.

I'll have another look on Monday if this still fails. Hopefully I'll be fresher then!

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

I think part of the problem is #3184

@AlanGriffiths
Copy link
Collaborator

@mattkae leave this a bit. I'm debugging the helper stuff

@AlanGriffiths
Copy link
Collaborator

@mattkae I have to dive into #3185 now, so good luck. (I've fixed some things, but the proof will be in CI. )

Some useful incantations for running the symbol (re)generation scripts:

cmake --build cmake-build-release --target regenerate-miral-symbols-map
cmake --build cmake-build-release --target regenerate-miral-debian-symbols

cmake --build cmake-build-release --target regenerate-miroil-symbols-map
cmake --build cmake-build-release --target check-miroil-symbols

@mattkae
Copy link
Contributor Author

mattkae commented Jan 12, 2024

@AlanGriffiths Ok i think this is good to go again. The symbols that were bumped are:

  • miral
  • miroil
  • mircommon
  • mirserver
  • mirwayland
  • mir-platform-graphics-*
  • mir-platform-rendering
  • mir-platform-input-*

Am I missing anything?

@mattkae mattkae requested a review from AlanGriffiths January 12, 2024 20:25
Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

Am I missing anything?

target_link_libraries(mirplatform
  PRIVATE
    mircommon
    ${MIR_PLATFORM_REFERENCES}
  PUBLIC
    PkgConfig::EPOXY
)

So, libmirplatform also needs an soname bump

@AlanGriffiths AlanGriffiths merged commit 34d87fd into feature/remove_mir_cookie Jan 16, 2024
23 of 24 checks passed
@AlanGriffiths AlanGriffiths deleted the feature/remove_mir_cookie_sonames branch January 16, 2024 17:33
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.

3 participants