Skip to content

chore(Makefile): Ensure targets are always run#3013

Merged
georglauterbach merged 2 commits intodocker-mailserver:masterfrom
polarathene:chore/makefile-always-run
Jan 21, 2023
Merged

chore(Makefile): Ensure targets are always run#3013
georglauterbach merged 2 commits intodocker-mailserver:masterfrom
polarathene:chore/makefile-always-run

Conversation

@polarathene
Copy link
Copy Markdown
Member

@polarathene polarathene commented Jan 21, 2023

Description

This commit is pulled out of #3012 and is explained in this PR comment:

it is not defined as a target like ALWAYS_RUN:, but just as .PHONY: ALWAYS_RUN :) This suffices to tell make to always run the target.

.PHONY: ALWAYS_RUN already exists in the Makefile. This PR just applies it more broadly as decided by @georglauterbach

NOTE: From what I have seen in Makefile docs for .PHONY this seems a bit of a different approach?

The targets seem to get appended to the .PHONY: target, rather than this usage of prefixing target values with ALWAYS_RUN? (a target dependency that is .PHONY) It seems like it may behave the same way, so perhaps it doesn't matter?

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

@georglauterbach @polarathene
added ALWAYS_RUN target Makefile recipies
8360d15 
This ensures the recipies are always run.
@polarathene polarathene added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Jan 21, 2023
@polarathene polarathene added this to the v12.0.0 milestone Jan 21, 2023
@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Jan 21, 2023

NOTE: From what I have seen in Makefile docs for .PHONY this seems a bit of a different approach?

The targets seem to get appended to the .PHONY: target, rather than this usage of prefixing target values with ALWAYS_RUN? (a target dependency that is .PHONY) It seems like it may behave the same way, so perhaps it doesn't matter?

I guess both work, I have seen both too.

casperklein
casperklein approved these changes Jan 21, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@casperklein casperklein left a comment •
edited
Loading

Choose a reason for hiding this comment

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

BTW: Even when .PHONY: ALWAYS_RUN and all other "ALWAYS_RUN" are removed, the function keeps the same. All targets are always run.
At least I couldn't identify any difference.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Jan 21, 2023

BTW: Even when .PHONY: ALWAYS_RUN and all other "ALWAYS_RUN" are removed, the function keeps the same. All targets are always run.

At least I couldn't identify any difference.

I thought exactly the same, and had already removed it at some point in time. But then, make told me the recipe was already up to date - very confusing! I don't reall know why, but this way, we're on the safe side.

As an alternative to make, I'd go with just, a proper command runner (remember: make is a build system!). But since everyone has make, I guess the transition would result in a bad development experience..

@georglauterbach georglauterbach merged commit 94a9d2a into docker-mailserver:master Jan 21, 2023
georglauterbach added a commit that referenced this pull request Jan 22, 2023
@polarathene @georglauterbach
added ALWAYS_RUN target Makefile recipies (#3013)
a9e4c1c 
This ensures the recipies are always run.

Co-authored-by: georglauterbach <[email protected]>
georglauterbach added a commit that referenced this pull request Jan 22, 2023
@polarathene @georglauterbach
added ALWAYS_RUN target Makefile recipies (#3013)
504b3c5 
This ensures the recipies are always run.

Co-authored-by: georglauterbach <[email protected]>
@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented Jan 22, 2023 •
edited
Loading

As an alternative to make, I'd go with just, a proper command runner (remember: make is a build system!). But since everyone has make, I guess the transition would result in a bad development experience..

I'd be open towards changing from make in a future release too. Most development using it is by us regular maintainers, and I'm not sure if it offers much value for anyone else not contributing?

Earthly has been on my radar, but I think that'd be more invasive of a change, and I'm not sure how beneficial it'd be as I think it may not support some features we're using with buildx (plus that's what our CI is setup with, switching would need to be worthwhile 😅 ). just looks pretty nice, plus it's in rust which is a win for me 😛

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Jan 22, 2023

I'd be open towards changing from make in a future release too. Most development using it is by us regular maintainers, and I'm not sure if it offers much value for anyone else not contributing?

You're probably right. @casperklein what's your take on this?

Earthly has been on my radar, but I think that'd be more invasive of a change, and I'm not sure how beneficial it'd be as I think it may not support some features we're using with buildx (plus that's what our CI is setup with, switching would need to be worthwhile 😅 ). just looks pretty nice, plus it's in rust which is a win for me 😛

I've never used earthly, I will have a look at it :) Just is something I use on a daily basis, and it works very well. I discovered it because I live Rust too 🙈

@casperklein
Copy link
Copy Markdown
Member

casperklein commented Jan 23, 2023

Pro make: well known and wide spread. And sufficient for our usage?
Pro something else: Could make things easier/more efficient.

Switching to something else should bring useful benefits. Otherwise I don't think it's worth to convert to something else. Also it's not that often, that our Makefile is modified. It's there and it does it's job.
But personally I don't care, if I have to run make, just or something else in the future 👍

@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented Jan 23, 2023 •
edited
Loading

Switching to something else should bring useful benefits. Otherwise I don't think it's worth to convert to something else.

Personally the Makefile has not been a fun experience for me with it's syntax and gotchas. So I think the benefit is improved maintenance with just (supposedly, I haven't looked into it much myself). That tends to be a good thing for new maintainers too if it would be cryptic to them 😅 (some parts aren't particularly google friendly, so a lot of sifting through git blame to grok what is going on)

Also it's not that often, that our Makefile is modified. It's there and it does it's job.

Fair. It's been stripped down quite a bit since it's first usage, but the syntax + usage perhaps more complicated to grok.

Our Makefile is at least easy to use and does work well for what we need. Only a small file that shouldn't need much more maintenance going forward, so I agree there's no major reason to switch away, but I'm open to it 👍

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Jan 23, 2023

I started a side-by-side comparison of the Makefile & the corresponding Justfile from just. Although just is way nicer, it occured to me that we would need this in our CI as well. This is inconvenient! I'd rather invest work in keeping the Makefile clean and short, even though I love just (I use it on a daily basis).

georglauterbach added a commit that referenced this pull request Jan 25, 2023
@georglauterbach @polarathene
feature: provide better rspamd suppport (#3016)
555fbb7 
* added options to toggle OpenDKIM & OpenDMARC

rspamd can provide DKIM signing and DMARC checking itself, so users
should be able to disable OpenDKIM & OpenDMARC. The default is left at
1, so users have to to opt-in when the want to disable the features.

* misc small enhancements

* adjusted start of rspamd

The order of starting redis + rspamd was reversed (now correct) and
rspamd now starts with the correct user.

* adjusted rspamd core configuration

The main configuration was revised. This includes AV configuration as
well as worker/proxy/controller configuration used to control the main
rspamd processes.

The configuration is not tested extensively, but well enough that I am
confident to go forward with it until we declare rspamd support as
stable.

* update & improve the documentation

* add tests

These are some initial tests which test the most basic functionality.

* tests(refactor): Improve consistency and documentation for test helpers (#3012)

* added `ALWAYS_RUN` target `Makefile` recipies (#3013)

This ensures the recipies are always run.

Co-authored-by: georglauterbach <[email protected]>

* adjusted rspamd test to refactored test helper functions

* improve documentation

* apply suggestions from code review (no. 1 by @polarthene)

Co-authored-by: Brennan Kinney <[email protected]>

* streamline heredoc (EOM -> EOF)

* adjust rspamd test (remove unnecessary run arguments)

Co-authored-by: Brennan Kinney <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@casperklein casperklein casperklein approved these changes

@georglauterbach georglauterbach georglauterbach approved these changes

Assignees

@georglauterbach georglauterbach

Labels

area/scripts kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Milestone

v12.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@polarathene @georglauterbach @casperklein