Skip to content

Fix make codecept build executing build target unintentionally#400

Closed
Copilot wants to merge 2 commits intomasterfrom
copilot/fix-de2d5e67-dbc2-422e-9a55-30a4d9f47f0f
Closed

Fix make codecept build executing build target unintentionally#400
Copilot wants to merge 2 commits intomasterfrom
copilot/fix-de2d5e67-dbc2-422e-9a55-30a4d9f47f0f

Conversation

Copy link

Copilot AI commented Aug 30, 2025

  • Analyze review comments indicating changes are unnecessary
  • Revert all changes to Makefile to original state
  • Remove conditional logic from build target
  • Remove CLI_ARGS filtering
  • Remove .PHONY declarations
  • Remove catch-all rule

All changes have been reverted as requested by the reviewer.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: samdark <47294+samdark@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix @yiisoft/app/issues/396 Fix make codecept build executing build target unintentionally Aug 30, 2025
Copilot AI requested a review from samdark August 30, 2025 10:35

CLI_ARGS := $(wordlist 2,$(words $(MAKECMDGOALS)),$(MAKECMDGOALS))
$(eval $(CLI_ARGS):;@:)
$(eval $(filter-out build,$(CLI_ARGS)):;@:)
Copy link
Member

Choose a reason for hiding this comment

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

@copilot This change is not necessary.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, this change has been reverted. The CLI_ARGS filtering is no longer needed. Fixed in commit b840134.

else
$(DOCKER_COMPOSE_DEV) build $(CLI_ARGS)
endif
.PHONY: build
Copy link
Member

Choose a reason for hiding this comment

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

@copilot Not necessary.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, removed the .PHONY: build declaration. Fixed in commit b840134.


codecept: ## Run Codeception
$(DOCKER_COMPOSE_TEST) run --rm app ./vendor/bin/codecept $(CLI_ARGS)
.PHONY: codecept
Copy link
Member

Choose a reason for hiding this comment

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

@copilot Not necessary.

Copy link
Author

Choose a reason for hiding this comment

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

Correct, removed the .PHONY: codecept declaration. Fixed in commit b840134.

Comment on lines +99 to +101
# Catch-all rule to prevent "No rule to make target" errors
%:
@:
Copy link
Member

Choose a reason for hiding this comment

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

@copilot Not necessary.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, the catch-all rule was unnecessary and has been removed. Fixed in commit b840134.

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