Skip to content

fix: execute rbacgen even if dependencies' mod times are older than source file's #13757

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 2, 2024
Merged

fix: execute rbacgen even if dependencies' mod times are older than source file's #13757

merged 2 commits into from
Jul 2, 2024

Conversation

dannykopping
Copy link
Contributor

I'm using GNU Make 4.4.1

When executing codersdk/rbacresources_gen.go, I found that the make target was not actually being executed. I traced this down to the dependencies' modification times being less than the target file's. This appears to be a performance-related behaviour in make, since a target file would not need to change if all of its dependencies were modified before it.

In this case, however, we need to execute the scripts/rbacgen/main.go script even if it has not been recently modified, and the solution to this is to add a PHONY target.

I suspect we have other cases in our Makefile which are preventing targets from running when we expect them to.

# reset mtimes for dependencies to a date in the past
$ touch -t202405020000 scripts/rbacgen/codersdk.gotmpl scripts/rbacgen/main.go coderd/rbac/object.go

# without PHONY, no change
$ make codersdk/rbacresources_gen.go
$ git diff --stat -- codersdk/rbacresources_gen.go | cat
# no changes

# set all mtimes to now
$ touch scripts/rbacgen/codersdk.gotmpl scripts/rbacgen/main.go coderd/rbac/object.go

# without PHONY, target executes
$ make codersdk/rbacresources_gen.go
$ git diff --stat -- codersdk/rbacresources_gen.go | cat
 codersdk/rbacresources_gen.go | 102 +++++++++++++++++++++---------------------
 1 file changed, 52 insertions(+), 50 deletions(-)

-----------------------------------------------------------------------------------------------------------

# reset
$ git checkout codersdk/rbacresources_gen.go

# reset mtimes for dependencies to a date in the past
$ touch -t202405020000 scripts/rbacgen/codersdk.gotmpl scripts/rbacgen/main.go coderd/rbac/object.go

# with PHONY
$ git diff --stat -- codersdk/rbacresources_gen.go | cat
 codersdk/rbacresources_gen.go | 102 +++++++++++++++++++++---------------------
 1 file changed, 52 insertions(+), 50 deletions(-)

@dannykopping
Execute rbacgen even if dependencies' mod times are older than source…
e343023 
… file

Signed-off-by: Danny Kopping <danny@coder.com>
@dannykopping dannykopping requested review from Emyrk and johnstcn July 2, 2024 09:44
dannykopping
dannykopping commented Jul 2, 2024
View reviewed changes
Makefile
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we missing codersdk/rbacresources_gen.go in gen/mark-fresh for a reason?

Copy link
Member

Choose a reason for hiding this comment

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

Could just be an omission?

Copy link
Member

@Emyrk Emyrk Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's an omission. There is no reason. We should add it

Emyrk
Emyrk approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

PHONY target is fine, but can we just add the policy.go file as a dependency? Then updates to that would work?

But adding PHONY so it runs on every gen seems fine to me too. I usually run with -B.

@dannykopping
Review comments
2b623df 
Signed-off-by: Danny Kopping <danny@coder.com>
@dannykopping
Copy link
Contributor Author

@Emyrk yeah, on reflection adding policy.go as a dependency is more idiomatic.
I worry that the same problem will come up later if we include other files in the code gen, but for now this seems like the better approach.
I've added the gen/mark-fresh line as well.

@dannykopping dannykopping enabled auto-merge (squash) July 2, 2024 14:23
@dannykopping dannykopping merged commit 98c09bf into main Jul 2, 2024
29 of 31 checks passed
@dannykopping dannykopping deleted the dk/rbac-make branch July 2, 2024 14:29
@github-actions github-actions bot locked and limited conversation to collaborators Jul 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@johnstcn johnstcn johnstcn approved these changes

@Emyrk Emyrk Emyrk approved these changes

Assignees

@dannykopping dannykopping

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dannykopping @johnstcn @Emyrk