Skip to content

Makefile: move SSL options into a block and refine rules #997

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
Merged
merged 1 commit into from
Dec 24, 2021
Merged

Makefile: move SSL options into a block and refine rules #997

merged 1 commit into from
Dec 24, 2021

Conversation

pizhenwei
Copy link
Contributor

@pizhenwei pizhenwei commented Oct 11, 2021 •
edited
Loading 

Makefile: move SSL options into a block and refine rules

Move SSL options into a block to make it easy to read, and change
global rules to make it easy to maintain. For the further step, it
gets extensible to add another type.

New version of the library building rule:
        static: $(STLIBNAME) $(SSL_STLIB)
        dynamic: $(DYLIBNAME) $(SSL_DYLIB)

Compare with the orignal version:
        dynamic: $(DYLIBNAME)
        static: $(STLIBNAME)
        ifeq ($(USE_SSL),1)
        dynamic: $(SSL_DYLIBNAME)
        static: $(SSL_STLIBNAME)
        endif

If we want to add a new type(Ex, RDMA), for the new version, we can
do like this:
        ########### RDMA options start ###############
        .....
        ########### RDMA options end   ###############

        static: $(STLIBNAME) $(SSL_STLIB) $(RDMA_STLIB)
        dynamic: $(DYLIBNAME) $(SSL_DYLIB) $(RDMA_DYLIB)

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

@pizhenwei
Copy link
Contributor Author

@itamarhaber Hi, I closed the original PR , and create a new PR against hiredis as you suggested.

@itamarhaber
Copy link
Member

itamarhaber commented Oct 12, 2021 •
edited
Loading

Thanks - the hiredis maintainers should pick it up from here.

@michael-grunder michael-grunder self-assigned this Oct 12, 2021
michael-grunder
michael-grunder reviewed Dec 22, 2021
View reviewed changes
Makefile Outdated
uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')

# Deps (use make dep to generate this)
alloc.o: alloc.c fmacros.h alloc.h
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will need to be moved below the all: target since right now running make without args only builds alloc.o

@michael-grunder
Copy link
Collaborator

Hi, thanks for the PR.

If you rebase your branch against master (and move the deps targets) I will test and get this merged.

@pizhenwei
Makefile: move SSL options into a block and refine rules
f74b081 
Move SSL options into two blocks to make it easy to read:
  1, first part: SSL variables part
  1, second part: SSL building rules part
and change global rules to make it easy to maintain. For the further
step, it gets extensible to add another type.

New version of the library building rule:
	static: $(STLIBNAME) $(SSL_STLIB)
	dynamic: $(DYLIBNAME) $(SSL_DYLIB)

Compare with the orignal version:
	dynamic: $(DYLIBNAME)
	static: $(STLIBNAME)
	ifeq ($(USE_SSL),1)
	dynamic: $(SSL_DYLIBNAME)
	static: $(SSL_STLIBNAME)
	endif

If we want to add a new type(Ex, RDMA), for the new version, we can
do like this:
	########### RDMA variables start ###############
	.....
	########### RDMA variables end   ###############

	static: $(STLIBNAME) $(SSL_STLIB) $(RDMA_STLIB)
	dynamic: $(DYLIBNAME) $(SSL_DYLIB) $(RDMA_DYLIB)

	########### RDMA building rules start ###############
	.....
	########### RDMA building rules end   ###############

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
@pizhenwei
Copy link
Contributor Author

Hi, thanks for the PR.

If you rebase your branch against master (and move the deps targets) I will test and get this merged.

Force updated, thanks for your suggestion.

@michael-grunder
Copy link
Collaborator

Looks like this breaks the tests in Windows, because it's unconditionally adding -lssl -lcrypto -lpthread regardless of the USE_SSL value.

This small modification on my fork seems to solve the problem.

If that looks OK to you I can include the patch when I merge.

@pizhenwei
Copy link
Contributor Author

Looks like this breaks the tests in Windows, because it's unconditionally adding -lssl -lcrypto -lpthread regardless of the USE_SSL value.

This small modification on my fork seems to solve the problem.

If that looks OK to you I can include the patch when I merge.

Thanks!

@michael-grunder michael-grunder merged commit f74b081 into redis:master Dec 24, 2021
@michael-grunder
Copy link
Collaborator

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-grunder michael-grunder michael-grunder left review comments

Assignees

@michael-grunder michael-grunder

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pizhenwei @itamarhaber @michael-grunder