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

improve Makefile #843

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ramessesii2
Copy link
Contributor

@ramessesii2 ramessesii2 commented Dec 29, 2024

  1. added jq to tools to be installed with make cli-install
  2. Solves this recurring issue:
make cli-install                                                                                                                                                                                                           
bash: line 1: /Users/ozymandias/work/hmc/bin/jq-1.7.1: No such file or directory
bash: line 1: /Users/ozymandias/work/hmc/bin/jq-1.7.1: No such file or directory
bash: line 1: /Users/ozymandias/work/hmc/bin/jq-1.7.1: No such file or directory
bash: line 1: /Users/ozymandias/work/hmc/bin/yq-v4.44.2: No such file or directory
bash: line 1: /Users/ozymandias/work/hmc/bin/jq-1.7.1: No such file or directory
bash: line 1: /Users/ozymandias/work/hmc/bin/jq-1.7.1: No such file or directory
bash: line 1: /Users/ozymandias/work/hmc/bin/jq-1.7.1: No such file or directory
bash: line 1: /Users/ozymandias/work/hmc/bin/yq-v4.44.2: No such file or directory

The Makefile is structured in a way that certain variable assignments at the top level utilize external tools like jq and yq through $() functions. These tools (jq and yq) are not installed when Make begins parsing the Makefile. Their installation is handled by the cli-install target, which occurs after the top-level variable assignments.

Fixes #512

Makefile Outdated Show resolved Hide resolved
@ramessesii2 ramessesii2 force-pushed the RAMESSES/makefile-fix branch from b5a6438 to 3a71599 Compare December 30, 2024 12:21
Copy link
Collaborator

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

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

Several notes:

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@ramessesii2 ramessesii2 force-pushed the RAMESSES/makefile-fix branch 3 times, most recently from e86b4ea to 077133e Compare January 3, 2025 05:11
    - add jq to tools to be installed with make cli-install
    - add check for unzip
    - refactor

Signed-off-by: Satyam Bhardwaj <[email protected]>
@ramessesii2 ramessesii2 force-pushed the RAMESSES/makefile-fix branch from 077133e to 2557f64 Compare January 6, 2025 14:10
Copy link
Collaborator

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

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

Target specific vars look bad
Please rebase

Comment on lines 479 to +482
$(FLUX_HELM_CRD): | $(EXTERNAL_CRD_DIR)
FLUX_HELM_VERSION ?= $(shell go mod edit -json | $(JQ) -r '.Require[] | select(.Path == "github.com/fluxcd/helm-controller/api") | .Version')
FLUX_HELM_NAME ?= helm
FLUX_HELM_CRD ?= $(EXTERNAL_CRD_DIR)/$(FLUX_HELM_NAME)-$(FLUX_HELM_VERSION).yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's justified here, since:

  1. Syntax looks incorrect
  2. You introduced duplication
  3. You didn't add jq to the target requirements (I assume that was the idea)

I suggest you just change jq -> $(JQ) in the global definitions and leave it as it were.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Makefile does not check for required binaries presence.
4 participants