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

Fix invalid version .so links in Makefile #16832

Merged
merged 2 commits into from
Nov 16, 2018

Conversation

clemensg
Copy link
Contributor

The links to the .so files previously used a ${settings.core_version.major} suffix for C, C++ and C#, which is incorrect.
Instead append the core major version for C, the cpp major version for C++ and the csharp major version for C# links.

This fixes an "error while loading shared libraries .. cannot open shared object file.." on an embedded Linux application which was linked dynamically to libgrpc++.

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,965,242      Total (=)      1,965,242

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,909,026      Total (>)     10,909,023

 No significant differences in binary sizes


@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@clemensg
Copy link
Contributor Author

clemensg commented Oct 18, 2018

Hi, I don't think that these three CI builds failed due to my changes. Is it possible to retry the build?

@clemensg
Copy link
Contributor Author

Gentle ping @nicolasnoble @jtattermusch

@clemensg
Copy link
Contributor Author

I reran generate_projects.sh and fixed-up the "Regenerate projects" commit in this PR to apply cleanly on top of the current master branch.

@clemensg
Copy link
Contributor Author

Over a month passed since I created this PR. No response? @jtattermusch @nicolasnoble

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,019,964      Total (=)      2,019,964

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,183,090      Total (<)     11,183,093

 No significant differences in binary sizes


@grpc-testing
Copy link

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

@jtattermusch
Copy link
Contributor

Known failures (not related to this PR):
#17218
#17138

@jtattermusch
Copy link
Contributor

Summary:
It seems that the SONAME of the shared libraries was adjusted in #10303 (which was only a partial fix for #10299), so the SONAME is now set to C++ major version but, the symlinks to that library are still created using the C-core version.

E.g.

$ objdump -p libgrpc++.so.1.17.0-dev | grep SONAME
SONAME               libgrpc++.so.1
# but a symlink libgrpc++.so.7  is created instead.

The PR seems to correctly fix the problem, thanks for the fix!

@@ -1370,7 +1370,7 @@
ifeq ($(SYSTEM),MINGW32)
$(Q) $(INSTALL) $(LIBDIR)/$(CONFIG)/lib${lib.name}$(SHARED_VERSION_${lang_to_var[lib.language]})-dll.a $(prefix)/lib/lib${lib.name}.a
else ifneq ($(SYSTEM),Darwin)
$(Q) ln -sf $(SHARED_PREFIX)${lib.name}$(SHARED_VERSION_${lang_to_var[lib.language]}).$(SHARED_EXT_${lang_to_var[lib.language]}) $(prefix)/lib/lib${lib.name}.so.${settings.core_version.major}
$(Q) ln -sf $(SHARED_PREFIX)${lib.name}$(SHARED_VERSION_${lang_to_var[lib.language]}).$(SHARED_EXT_${lang_to_var[lib.language]}) $(prefix)/lib/lib${lib.name}.so.${settings.get(lang_to_var[lib.language].lower() + '_version').major}
Copy link
Contributor

@jtattermusch jtattermusch Nov 16, 2018

Choose a reason for hiding this comment

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

FTR, the expression ${settings.get(lang_to_var[lib.language].lower() + '_version').major} is taken from https://github.com/grpc/grpc/pull/10303/files#diff-7b747c345348de73c8376cf79e5958f3R1562 (and that seems correct).

@jtattermusch jtattermusch self-assigned this Nov 16, 2018
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang/c++ release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants