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.

Sign up for GitHub

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

Jump to bottom

Makefile.template: fixed variables names, and added a missing one #9333

Merged
merged 2 commits into from
Jan 13, 2017
Merged

Makefile.template: fixed variables names, and added a missing one #9333

merged 2 commits into from
Jan 13, 2017

Conversation

emmenlau
Copy link
Contributor

Required for MINGW or MSYS2 compilation

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

13 similar comments
@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@jtattermusch jtattermusch self-requested a review January 12, 2017 15:42
@jtattermusch jtattermusch self-assigned this Jan 12, 2017
@jtattermusch
Copy link
Contributor

Can you please elaborate the description of the PR for future reference?

@jtattermusch
Copy link
Contributor

this is ok to test

jtattermusch
jtattermusch reviewed Jan 12, 2017
View reviewed changes
templates/Makefile.template
@@ -1303,7 +1303,7 @@
% if not lib.get('external_deps', None):
$(E) "[INSTALL] Installing $(SHARED_PREFIX)${lib.name}$(SHARED_VERSION_${lang_to_var[lib.language]}).$(SHARED_EXT_${lang_to_var[lib.language]})"
$(Q) $(INSTALL) -d $(prefix)/lib
$(Q) $(INSTALL) $(LIBDIR)/$(CONFIG)/$(SHARED_PREFIX)${lib.name}$(SHARED_VERSION_${lang_to_var[lib.language]}).$(SHARED_EXT_${lang_to_var[lib.language]}) $(prefix)/lib/$(SHARED_PREFIX)${lib.name}$(SHARED_VERSION).$(SHARED_EXT_${lang_to_var[lib.language]})
$(Q) $(INSTALL) $(LIBDIR)/$(CONFIG)/$(SHARED_PREFIX)${lib.name}$(SHARED_VERSION_${lang_to_var[lib.language]}).$(SHARED_EXT_${lang_to_var[lib.language]}) $(prefix)/lib/$(SHARED_PREFIX)${lib.name}$(SHARED_VERSION_${lang_to_var[lib.language]}).$(SHARED_EXT_${lang_to_var[lib.language]})
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you forget to include changes to the regenerated Makefile (only changing template here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm sorry that I was not able to correctly create a Makefile from the Makefile.template. Thanks to your
hint (installing golang in Ubuntu) it works now. Should I add the changes to the Makefile?

templates/Makefile.template
@@ -1558,11 +1558,11 @@
$(E) "[LD] Linking $@"
$(Q) mkdir -p `dirname $@`
ifeq ($(SYSTEM),Darwin)
$(Q) ${ld} ${ldflags} -L$(LIBDIR)/$(CONFIG) -install_name $(SHARED_PREFIX)${lib.name}$(SHARED_VERSION).$(SHARED_EXT_${lang_to_var[lib.language]}) -dynamiclib -o ${out_libbase}.$(SHARED_EXT_${lang_to_var[lib.language]}) ${common}${link_libs}
$(Q) ${ld} ${ldflags} -L$(LIBDIR)/$(CONFIG) -install_name $(SHARED_PREFIX)${lib.name}$(SHARED_VERSION_${lang_to_var[lib.language]}).$(SHARED_EXT_${lang_to_var[lib.language]}) -dynamiclib -o ${out_libbase}.$(SHARED_EXT_${lang_to_var[lib.language]}) ${common}${link_libs}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change things for Darwin? I am not sure I follow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it based on the PR description.

@emmenlau
Copy link
Contributor Author

Sorry I should have elaborated more! The Makefile.template references the variable
$(SHARED_VERSION) that AFAIK is undefined. The variable AFAIK should be
correctly named $(SHARED_VERSION_${lang_to_var[lib.language]}). I've compared
the Makefile to older versions and I think $(SHARED_VERSION) has been
deprecated, but was not consistently replaced. This is just a guess though.

However the whole discussion is a bit academic, since on most platforms the variables
$(SHARED_VERSION_XXX) are defined to empty. So its just a fix for consistency, it
should not actually change the build result.

Finally in line 1523 the $(SHARED_VERSION_' + lang_to_var[dep_lib.language] + ')
was missing for mingw. I added it there.

jtattermusch
jtattermusch approved these changes Jan 13, 2017
View reviewed changes
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

@jtattermusch
Copy link
Contributor

Known test failures:
#9150
#8989

@jtattermusch jtattermusch merged commit 9752569 into grpc:master Jan 13, 2017
@emmenlau emmenlau deleted the emmenlau_fix_some_variable_names branch January 13, 2017 09:03
@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jtattermusch jtattermusch jtattermusch approved these changes

Assignees

@jtattermusch jtattermusch

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@emmenlau @grpc-kokoro @jtattermusch @googlebot