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

NO-JIRA: chore(Makefile): replace ^\t (that's the default .RECIPEPREFIX) with > #806

Closed
wants to merge 1 commit into from

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Dec 12, 2024

Description

Turns out that progress is not as fast as it could be because developers have difficulties using the \t character in Makefile correctly. In order to accelerate the development, we should stop using the difficult old-school significant-whitespace syntax and instead we should switch to use the > character, that is fit for the sensibilities of the 21th century.

future: past
> ONWARDS!

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci openshift-ci bot requested review from jstourac and paulovmr December 12, 2024 11:01
Copy link
Contributor

openshift-ci bot commented Dec 12, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jiridanek. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jiridanek jiridanek added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 12, 2024
@jiridanek jiridanek changed the title NO-JIRA: chore(Makefile): replace ^\t (that's the .RECIPEPREFIX) with > NO-JIRA: chore(Makefile): replace ^\t (that's the default .RECIPEPREFIX) with > Dec 12, 2024
@jstourac
Copy link
Member

I'm not sure I like the > style... I would have to get used to it. But if that means we can use just spaces and not being confused around, I think I can live with that. I'll give lgtm, but this should definitely be agreed by wider team members first.

/lgtm

@jiridanek
Copy link
Member Author

jiridanek commented Dec 12, 2024

what's @harshad16 gonna say when all tabs are gone after he comes from pto, tho?

@jiridanek
Copy link
Member Author

/override ci/prow/images

Copy link
Contributor

openshift-ci bot commented Dec 12, 2024

@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/images

In response to this:

/override ci/prow/images

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@andyatmiami
Copy link
Contributor

andyatmiami commented Dec 13, 2024

I am with @jstourac in that I feel the > is a bit of an odd choice.... but, while I have no personally yet experienced the pain, I can appreciate the motivate outlined in https://tech.davis-hansson.com/p/make/.

I would be just as happy to leave it with ^\t - but also have no problem adopting > if that is the direction the team wants to go.

However, this does potentially lead to some pain/vigilance when we sync from upstream, right? any changes in the Makefile made upstream will require then manual intervention from us to add the prefix character?

Whoops, forgot what repo I was in 😜

That being said... in a similar vein.. how will we approach Makefile in the other repos we manage? Is this a change we wanna rollout consistently across repos? At which point... doing that on repos with an upstream - as I incorrectly attributed to this repo - leads to at minimum minor annoyance/overhead.

@jiridanek
Copy link
Member Author

I am with @jstourac in that I feel the > is a bit of an odd choice

no disagreement there, but almost anything is better than the default significant-whitespace syntax, imo

c.f. https://en.wikipedia.org/wiki/Whitespace_(programming_language) for an extreme case of significant-whitespace in action

Copy link
Contributor

openshift-ci bot commented Dec 13, 2024

@jiridanek: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/images fef834b link true /test images

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@caponetto
Copy link
Contributor

I'm not sure I like the > style... I would have to get used to it. But if that means we can use just spaces and not being confused around, I think I can live with that. I'll give lgtm, but this should definitely be agreed by wider team members first.

/lgtm

+1 for Jan's comment but I'm not against it, so
/lgtm

It'd be nice if we could have a format checker on every commit/PR to guarantee the rules we agree on.

@jiridanek
Copy link
Member Author

Discussed in office-hours. Let's not do this, because

  1. we want to remove the long shell scripts from Makefile anyways, and when the Makefile is simple this is not nearly as beneficial
  2. other Makefiles in other repos are still using tabs, and we can't change those, because they come from kubeflow/kubeflow, so doing this would lead to confusion
  3. intelliJ does not support changing the recipeprefix

@jiridanek jiridanek closed this Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants