Skip to content

Update variables override mechanism to append#368

Merged
SuperSandro2000 merged 2 commits intomainfrom
update_variables_overrides
Sep 17, 2025
Merged

Update variables override mechanism to append#368
SuperSandro2000 merged 2 commits intomainfrom
update_variables_overrides

Conversation

@stanislav-zaprudskiy
Copy link
Copy Markdown
Contributor

@stanislav-zaprudskiy stanislav-zaprudskiy commented Sep 10, 2025

The updated mechanism brought by #362 behaves differently to the behavior in #352 in a way that it prepends to the variable not appends to it when overriding with environment - which makes the overall feature of "adding to a variable" less useful.

The PRs reverts #362 and configures Makefile to not produce warnings on unused variables by default:

  • Remove MAKEFLAGS=--warn-undefined-variables
    This is not generally required, and could manually be added as command
    line argumen when needed e.g. to debug a Makefile.

    `--warn-undefined-variables'

    Issue a warning message whenever make sees a reference to an undefined
    variable. This can be helpful when you are trying to debug makefiles
    which use variables in complex ways.


A practical example. The following configuration works perfectly fine both in regular CI workflow and on local machine:

variables:
  GO_TESTFLAGS: "-short -count=1 -v -failfast"

but making it execute with -short=false (e.g. in a nightly CI workflow) requires providing all other arguments as well, as += prepends to a variable, not appends to it as seen below.


Given the following Makefile:

GO_TESTFLAGS += -short=true

foo:
	@echo flags are \"$(GO_TESTFLAGS)\"

it produces:

GO_TESTFLAGS="-short=false" make
flags are "-short=false -short=true"

Whereas a Makefile updated to

GO_TESTFLAGS := -short=true $(GO_TESTFLAGS)
...

produces:

GO_TESTFLAGS="-short=false" make
flags are "-short=true -short=false"

which would result to an overridden argument when used with e.g. go test.

This is not generally required, and could manually be added as command
line argumen when needed e.g. to debug a Makefile.

> `--warn-undefined-variables'
>
> Issue a warning message whenever make sees a reference to an undefined
> variable. This can be helpful when you are trying to debug makefiles
> which use variables in complex ways.
@github-actions
Copy link
Copy Markdown
Contributor

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/sapcc/go-makefile-maker/internal/makefile 8.02% (+0.02%) 👍

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/sapcc/go-makefile-maker/internal/makefile/makefile.go 0.00% (ø) 195 (-1) 0 195 (-1)

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

@stanislav-zaprudskiy stanislav-zaprudskiy changed the title update variables overrides Update variables override mechanism to append Sep 10, 2025
@stanislav-zaprudskiy stanislav-zaprudskiy marked this pull request as ready for review September 10, 2025 06:41
Copy link
Copy Markdown
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

LGTM

@SuperSandro2000 SuperSandro2000 merged commit 5ce89a8 into main Sep 17, 2025
8 checks passed
@SuperSandro2000 SuperSandro2000 deleted the update_variables_overrides branch September 17, 2025 13:17
# SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company
# SPDX-License-Identifier: Apache-2.0

MAKEFLAGS=--warn-undefined-variables
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I object to this being removed. Let's please figure out a way to bring this back, since this has already caught bugs in the past.

@majewsky
Copy link
Copy Markdown
Contributor

majewsky commented Sep 23, 2025

Reverting this entire PR via #373 because it breaks the documented behavior. make GO_TESTFLAGS="-short=false" instead of GO_TESTFLAGS="-short=false" make would have produced the desired effect, and that was already documented.

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.

4 participants