Skip to content

Makefile: Use ?= for initial variable definitions #725

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
mergify merged 1 commit into from
Jun 30, 2022
Merged

Makefile: Use ?= for initial variable definitions #725

mergify merged 1 commit into from
Jun 30, 2022

Conversation

@anoopcs9
Copy link
Collaborator

@anoopcs9 anoopcs9 commented Jun 29, 2022

:= is not capable of accepting values(for variables) from execution environment. Therefore replace := with ?= for completeness.

@anoopcs9 anoopcs9 added the no-API This PR does not include any changes to the public API of a go-ceph package label Jun 29, 2022
@phlogistonjohn
Copy link
Collaborator

phlogistonjohn commented Jun 29, 2022

Yeah, they don't allow being changed from the environment, but they do accept user input on the CLI. I do it all the time. I'm not opposed to the change but I am curious about why we should be taking these variables from the environment rather than the CLI?

@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Jun 29, 2022

I'm not opposed to the change but I am curious about why we should be taking these variables from the environment rather than the CLI?

I happened to notice this shortcoming while trying out various Makefile targets in the following format:

# VARIABLE1=<value> VARIABLE2=<value> . . . make <target>

I tend to use the above format more often which I think is something we could provide support for without any harm.

@ansiwen
Copy link
Collaborator

ansiwen commented Jun 30, 2022 •
edited
Loading 

# VARIABLE1=<value> VARIABLE2=<value> . . . make <target>

I tend to use the above format more often which I think is something we could provide support for without any harm.

But

$ make VARIABLE1=<value> VARIABLE2=<value> ... <target>

would work, right?

However, what I like about the change, is that I could enable caching and stuff in my env and don't have to explicitly type it every time. So I'm in. :-)

Oh wait, the USE_CACHE is not part of this? Hmm, could you add that, @anoopcs9? 😬

@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Jun 30, 2022 

# VARIABLE1=<value> VARIABLE2=<value> . . . make <target>

I tend to use the above format more often which I think is something we could provide support for without any harm.

But

$ make VARIABLE1=<value> VARIABLE2=<value> ... <target>

would work, right?

Yes.

Oh wait, the USE_CACHE is not part of this? Hmm, could you add that, @anoopcs9? 😬

Sure.

@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Jun 30, 2022 •
edited
Loading

Oh wait, the USE_CACHE is not part of this? Hmm, could you add that, @anoopcs9? 😬

Sure.

It looks like the initial definition for USE_CACHE is absent and thus value set from env should be honoured through the following statements:

ifneq ($(USE_CACHE),)
        GOCACHE_VOLUME := -v test_ceph_go_cache:/go
endif

@anoopcs9 anoopcs9 force-pushed the makefile-short-hand branch from 880586e to 6575eca Compare June 30, 2022 11:27
@ansiwen
Copy link
Collaborator

ansiwen commented Jun 30, 2022

It looks like the initial definition for USE_CACHE is absent and thus value set from env should be honoured through the following statements:

Oh right! I never bothered to try. 😅

ansiwen
ansiwen reviewed Jun 30, 2022
View reviewed changes
Copy link
Collaborator

@ansiwen ansiwen left a comment

Choose a reason for hiding this comment

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

I'm fine with this, but I'll let @phlogistonjohn approve.

phlogistonjohn
phlogistonjohn approved these changes Jun 30, 2022
View reviewed changes
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

Works for me.

@phlogistonjohn
Copy link
Collaborator

phlogistonjohn commented Jun 30, 2022

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Jun 30, 2022

rebase

✅ Branch has been successfully rebased

@ktdreyer ktdreyer force-pushed the makefile-short-hand branch from 6575eca to 72a8a76 Compare June 30, 2022 17:29
@anoopcs9 @mergify
Makefile: Use ?= for initial variable definitions
6e217a8 
:= is not capable of accepting values(for variables) from execution
environment. Therefore replace := with ?= for completeness.

Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
@ktdreyer ktdreyer force-pushed the makefile-short-hand branch from 72a8a76 to 6e217a8 Compare June 30, 2022 18:02
@mergify mergify bot merged commit 1589706 into ceph:master Jun 30, 2022
@anoopcs9 anoopcs9 deleted the makefile-short-hand branch July 6, 2022 12:15
@anoopcs9 anoopcs9 mentioned this pull request Jul 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ansiwen ansiwen ansiwen left review comments

@phlogistonjohn phlogistonjohn phlogistonjohn approved these changes

Assignees

No one assigned

Labels

no-API This PR does not include any changes to the public API of a go-ceph package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anoopcs9 @phlogistonjohn @ansiwen