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: Convert to Makefile-style and correct wrong usage #289

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

PaulYuuu
Copy link
Contributor

In a Makefile, when we use doule "$", we should make sure the variable
is present in the enviornment. However, in the current design, most
variables are Makefile variables, so that shell failed to handle them.

Signed-off-by: Yihuang Yu [email protected]

In a Makefile, when we use doule "$", we should make sure the variable
is present in the enviornment. However, in the current design, most
variables are Makefile variables, so that shell failed to handle them.

Signed-off-by: Yihuang Yu <[email protected]>
@PaulYuuu
Copy link
Contributor Author

PaulYuuu commented Apr 18, 2024

The issue is we mixed the usage between shell-style and makefile-style variables. Let me show the reproducer:

$ cat Makefile
ARCH := $(shell uname -m)

ROOTLESS_AUTH_JSON=${XDG_RUNTIME_DIR}/containers/auth.json
ROOTFUL_AUTH_JSON=/run/containers/0/auth.json
NONLINUX_AUTH_JSON=${HOME}/.config/containers/auth.json
AUTH_JSON ?=

ifneq ("$(wildcard $(NONLINUX_AUTH_JSON))","")
        AUTH_JSON=$(NONINUX_AUTH_JSON);
else ifneq ("$(wildcard $(ROOTLESS_AUTH_JSON))","")
        AUTH_JSON=$(ROOTLESS_AUTH_JSON);
else ifneq ("$(wildcard $(ROOTFUL_AUTH_JSON))","")
        AUTH_JSON=$(ROOTFUL_AUTH_JSON);
endif

.PHONY: check_var

check_var:
	@echo ARCH is: $${ARCH:+--arch $${ARCH}}
	@echo AUTH_JSON is: $${AUTH_JSON:+-v $${AUTH_JSON}:/run/containers/0/auth.json}

$ make -f Makefile
ARCH is:
AUTH_JSON is:

$ make -f Makefile AUTH_JSON=${XDG_RUNTIME_DIR}/containers/auth.json
ARCH is:
AUTH_JSON is: -v /run/user/1001/containers/auth.json:/run/containers/0/auth.json

@PaulYuuu
Copy link
Contributor Author

I think we have 2 ways to fix this, the first one is to export global makefile variables to env, another one is to use makefile-style variables in targets.

$ cat Makefile
ARCH := $(shell uname -m)

ROOTLESS_AUTH_JSON=${XDG_RUNTIME_DIR}/containers/auth.json
ROOTFUL_AUTH_JSON=/run/containers/0/auth.json
NONLINUX_AUTH_JSON=${HOME}/.config/containers/auth.json
AUTH_JSON ?=

ifneq ("$(wildcard $(NONLINUX_AUTH_JSON))","")
        AUTH_JSON=$(NONINUX_AUTH_JSON)
else ifneq ("$(wildcard $(ROOTLESS_AUTH_JSON))","")
        AUTH_JSON=$(ROOTLESS_AUTH_JSON)
else ifneq ("$(wildcard $(ROOTFUL_AUTH_JSON))","")
        AUTH_JSON=$(ROOTFUL_AUTH_JSON)
endif
export AUTH_JSON

.PHONY: check_var

check_var:
	@echo ARCH is: $(ARCH:%=--arch %)
	@echo AUTH_JSON is: $${AUTH_JSON:+-v $${AUTH_JSON}:/run/containers/0/auth.json}

$ make -f Makefile ARCH=aarch64
ARCH is: --arch aarch64
AUTH_JSON is: -v /run/user/1001/containers/auth.json:/run/containers/0/auth.json
$ make -f Makefile
ARCH is: --arch x86_64
AUTH_JSON is: -v /run/user/1001/containers/auth.json:/run/containers/0/auth.json

@rhatdan
Copy link
Member

rhatdan commented Apr 18, 2024

LGTM

@rhatdan rhatdan merged commit b2d678d into containers:main Apr 18, 2024
1 check passed
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.

2 participants