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

build: ensure Makefile driven dependencies are up to date #13623

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented Sep 19, 2024

Motivation

As a developer when the Makefile changes the version of a program that it uses or another dependency I would like to use the new version of that program or dependency

Created in response to https://cloud-native.slack.com/archives/C0510EUH90V/p1726666701563199

Modifications

For all make targets that specify their version in the Makefile specify the Makefile as a dependency so they will get refetched if the Makefile changes. This is crude as it will refetch all dependencies regardless of whether they have changed or not, so touching the Makefile will cause a slower rebuild. Correctness trumps speed here in my opinion.

Add -y to the apt get call installing clang-format so that it becomes non-interactive. It's annoying to have to type y when this runs anyway.

Use the special Makefile variable $@ instead of repeating the target name explicitly in the rules I was touching where it was relevant - following the DRY principle.

Most annoyingly recurl.sh doesn't do any work if the target file exists, so remove the file before fetching in the case where that is used so we get an updated version. This foxed me for a while as this was the file causing the original issue linked above in slack.

Verification

Locally make codegen successfully on a dirty checkout which had run codegen prior to #13413.

Notes to approvers

This is pushed to argoproj so you can make direct edits if you feel that's appropriate for collective ownership of fixing this.

Signed-off-by: Alan Clucas [email protected]

@Joibel Joibel assigned Joibel and unassigned Joibel Sep 19, 2024
@Joibel Joibel added the area/build Build or GithubAction/CI issues label Sep 19, 2024
@terrytangyuan terrytangyuan changed the title fix(build): ensure Makefile driven dependencies are up to date build: ensure Makefile driven dependencies are up to date Sep 19, 2024
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

I changed the title. Otherwise LGTM

@Joibel
Copy link
Member Author

Joibel commented Sep 19, 2024

I changed the title. Otherwise LGTM

Thanks, I forget about these other titles. I'll try to remember.

I think the test-functional failures are genuine. I'm investigating.

@Joibel Joibel enabled auto-merge (squash) September 19, 2024 13:04
@Joibel Joibel merged commit 4afcbc8 into main Sep 19, 2024
26 of 27 checks passed
@Joibel Joibel deleted the makefile-fix-deps branch September 19, 2024 13:34
@agilgur5
Copy link
Member

agilgur5 commented Sep 20, 2024

I changed the title. Otherwise LGTM

Thanks, I forget about these other titles. I'll try to remember.

As I mentioned in #13607 (comment), the fix prefix could be fine if we plan to backport the PR, since the cherry-pick.sh script filters on prefix. That's really the main consideration.
We often don't backport build changes, but sometimes do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants