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

Tidies up Makefile: fixes default target, remove unused ones and makes phony targets to by be truly phony #299

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

m1kola
Copy link
Member

@m1kola m1kola commented Jul 27, 2023

Description

  • Fixes default target. It looks like target all was previously define first to implicitly set build to be default target, but since introduction include above the all target - now BINGO is default target. This change explicitly sets build to be default.
  • Sets several targets as phony as they are not targeting a file and moves .PHONY closer to target definition for better visibility.
  • Removes unused wait target

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #299 (c40447e) into main (14c9a3d) will decrease coverage by 3.70%.
The diff coverage is n/a.

❗ Current head c40447e differs from pull request most recent head 0260151. Consider uploading reports for the commit 0260151 to get more accurate results

@@            Coverage Diff             @@
##             main     #299      +/-   ##
==========================================
- Coverage   80.69%   77.00%   -3.70%     
==========================================
  Files          18       15       -3     
  Lines         803      687     -116     
==========================================
- Hits          648      529     -119     
- Misses        112      129      +17     
+ Partials       43       29      -14     
Flag Coverage Δ
e2e ?
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 5 files with indirect coverage changes

@m1kola m1kola marked this pull request as ready for review July 27, 2023 16:11
@m1kola m1kola requested a review from a team as a code owner July 27, 2023 16:11
@m1kola m1kola changed the title Tidies up Makefile: fixes default target, remove unused ones and makes phony targets to by be trully phony Tidies up Makefile: fixes default target, remove unused ones and makes phony targets to by be truly phony Jul 28, 2023
@ncdc
Copy link
Member

ncdc commented Jul 28, 2023

re the unused wait target - do we know if anyone was calling make wait as part of their workflow?

* Fixes default target. It looks like target `all`
  was previously define first to implicitly set `build`
  to be default target, but since introduction `include`
  above the `all` target - now `BINGO` is default target.
  This change explicitly sets `build` to be default.
* Sets several targets as phony as they are not targeting
  a file and moves `.PHONY` closer to target definition
  for better visibility.
* Removes unused `wait` target

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
@m1kola
Copy link
Member Author

m1kola commented Jul 28, 2023

@ncdc I believe this was introduced to be used by run target. At some point wait was removed from run as a dependency. So wait seems to be obsolete.

I think it is not documented anywhere nor it appears in make help. I think it is relatively safe to remove it.


I rebased the PR to fix conflicts. (Edit: I pushed but GitHub doesn't seem to show changes just yet. Edit 2: all good now)

@ncdc ncdc added this pull request to the merge queue Jul 28, 2023
Merged via the queue into operator-framework:main with commit 1a431d9 Jul 28, 2023
7 checks passed
@m1kola m1kola deleted the makefile-tidyup branch July 28, 2023 21:51
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.

None yet

2 participants