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

Avoid unnecessary rebuilds when building for Linux #618

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

egrimley-arm
Copy link
Contributor

@egrimley-arm egrimley-arm commented May 31, 2023

To demonstrate the problem that this PR is intended to fix:

  • cd veracruz/workspaces
  • make clean
  • make linux
  • make linux

There will be several invocations of cargo build, but if any significant work happens after the second make linux, that's a bug.


  • transport-protocol/build.rs: Put generated file in tmp dir and rename.

This is to avoid unnecesssary rebuilds when several other crates have a path dependency on transport-protocol. Without this change, one of the crates getting rebuilt causes the timestamp on the generated file (transport_protocol.rs) to get updated, which makes the other crates get rebuilt by cargo build next time.

A neater solution might be to put the generated file under OUT_DIR, but currently that seems to be difficult.
(See stepancheg/rust-protobuf#673.)

  • examples/data-generators/: Fix dependencies on directories.

Also various minor simplifications and tidying.

A dependency on a directory causes unnecessary rebuilds as make looks at the modification time of the directory, which gets updated as files are created in the directory, so the directory ends up being newer than some of the target files, forcing a rebuild. An order-only prerequisite is used to avoid this problem.

List of changes:

  • Use an "order-only prerequisite" to create $(TARGET_DIR).
  • Remove all uses of .SECONDEXPANSION and notdir.
  • Remove all "static pattern rules".
  • Use "grouped targets" where appropriate.
  • Update sources for external data.
  • Add moving-average-*/apple_prices.csv.
  • Remove trailing / from value of TARGET_DIR.
  • Use quick-clean: clean.
  • Remove trailing spaces.

@gbryant-arm
Copy link
Contributor

Cool!
Is this specific to Linux?
How much build time is this supposed to save roughly? make linux takes 1s on my machine without this

@egrimley-arm
Copy link
Contributor Author

I was only looking for problems in the Linux build but I expect these changes also affect builds for other targets.

The percentage change in the build time from a fresh checkout is probably rather small, but rebuilds after changing certain components might be changed more significantly.

@mathias-arm
Copy link
Contributor

Why not use the | (https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html):

$(TARGET_DIR_DATS): %.dat : $$(notdir $$@) | $(TARGET_DIR)

@egrimley-arm
Copy link
Contributor Author

Unless you've got several recipes that need to create the same directory, doesn't that just make the Makefile more complicated?

@mathias-arm mathias-arm self-requested a review June 8, 2023 15:53
@mathias-arm
Copy link
Contributor

Unless you've got several recipes that need to create the same directory, doesn't that just make the Makefile more complicated?

In several of those Makefiles there are several targets that depend on the same directory (you missed some of them). It would make the diff smaller. Given the amount of similarity between the Makefiles there would be opportunities for factorization.

@egrimley-arm
Copy link
Contributor Author

I'll try that then ...

@egrimley-arm egrimley-arm marked this pull request as draft June 8, 2023 17:02
This is to avoid unnecesssary rebuilds when several other crates have
a path dependency on transport-protocol. Without this change, one of
the crates getting rebuilt causes the timestamp on the generated file
("transport_protocol.rs") to get updated, which makes the other crates
get rebuilt by "cargo build" next time.

A neater solution might be to put the generated file under OUT_DIR,
but currently that seems to be difficult.
Also various minor simplifications and tidying.

A dependency on a directory causes unnecessary rebuilds as make
looks at the modification time of the directory, which gets updated
as files are created in the directory, so the directory ends up
being newer than some of the target files, forcing a rebuild.
An order-only prerequisite is used to avoid this problem.

List of changes:

- Use an "order-only prerequisite" to create $(TARGET_DIR).
- Remove all uses of .SECONDEXPANSION and notdir.
- Remove all "static pattern rules".
- Use "grouped targets" where appropriate.
- Update sources for external data.
- Add moving-average-*/apple_prices.csv.
- Remove trailing / from value of TARGET_DIR.
- Use "quick-clean: clean".
- Remove trailing spaces.
@egrimley-arm
Copy link
Contributor Author

There are now quite a lot of new changes in examples/data-generators/*/Makefile.

@egrimley-arm egrimley-arm marked this pull request as ready for review June 13, 2023 09:28
@egrimley-arm egrimley-arm merged commit e501bb9 into veracruz-project:main Jun 13, 2023
@egrimley-arm egrimley-arm deleted the pr-make-make branch June 13, 2023 09:28
@mathias-arm mathias-arm added the build-process Something related to the Veracruz build process label Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-process Something related to the Veracruz build process
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants