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

fix: make check always compiles dependencies #164

Merged
merged 3 commits into from
Aug 4, 2023

Conversation

steve-chavez
Copy link
Contributor

Problem

Make is supposed to only compile dependencies when they change but make check always compiles them, making the test suite run slow.

$ make check
cc ... build/mpc.o

$ make check
cc ... build/mpc.o

The same occurs for other targets like build/test-dynamic and build/test-static.

This happens because these targets have the $(DIST) directory as a dependency, and directories always change whenever members are added, removed or renamed.

See https://stackoverflow.com/a/4445262/4692662

Solution

Fix this by changing the $(DIST) directory dependency to a $(DIST)/.dirstamp file.

Now make check compiles dependencies only once.

Make is supposed to only compile dependencies when they change but
`make check` always compiles them, making the test suite run slow.

```
$ make check
cc ... build/mpc.o

$ make check
cc ... build/mpc.o
```

The same occurs for other targets like `build/test-dynamic` and
`build/test-static`.

This happens because these targets have the `$(DIST)` directory as a
dependency, and directories always change whenever members are added,
removed or renamed.

Fix this by changing the `$(DIST)` directory dependency to a
`$(DIST)/.dirstamp` file.

Now `make check` compiles dependencies only once.
@HalosGhost HalosGhost self-assigned this Aug 4, 2023
@HalosGhost
Copy link
Collaborator

Yeah, that's my fault. I tend to solve this a different way these days, but I like your fix better than completely changing the mechanism.

Concept ACK. Will test locally.

Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

I have a couple of small suggestions, but the core patch avoids needless rebuilds on make check, as desired.

.gitignore Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@HalosGhost HalosGhost self-requested a review August 4, 2023 02:52
Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

Tested ACK. This looks good and works as-expected.

I think this is ready for rebase/fixup and merge.

@orangeduck orangeduck merged commit 6912695 into orangeduck:master Aug 4, 2023
1 check passed
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.

3 participants