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

Only remake $(TARGET_BASE).elf el al. if necessary #1223

Merged
merged 1 commit into from Jan 15, 2018

Conversation

@jablko
Copy link
Contributor

commented Jan 1, 2017

Commit ee1d29e made it possible to run program-dfu and friends without
first running make all, but it also made it so the $(TARGET_BASE).elf
recipe is constantly executed, whether it needs it or not.

The problem is that currently, $(MAKE_DEPENDENCIES) is phony, so real
targets shouldn't depend on that [1]. Phony prerequisites are never up
to date, so neither is anything that depends on them, and make rebuilds
anything that isn't up to date.

One solution is to move the phony prerequisites from the real targets to
program-dfu, which is itself phony. That's how the all target worked
before ee1d29e.

[1] https://www.gnu.org/software/make/manual/html_node/Phony-Targets


Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation
  • Add to CHANGELOG.md after merging (add links to docs and issues)
Only remake $(TARGET_BASE).elf el al. if necessary
Commit ee1d29e made it possible to run program-dfu and friends without
first running `make all`, but it also made it so the $(TARGET_BASE).elf
recipe is constantly executed, whether it needs it or not.

The problem is that currently, $(MAKE_DEPENDENCIES) is phony, so real
targets shouldn't depend on that [1]. Phony prerequisites are never up
to date, so neither is anything that depends on them, and make rebuilds
anything that isn't up to date.

One solution is to move the phony prerequisites from the real targets to
program-dfu, which is itself phony. That's how the `all` target worked
before ee1d29e.

[1] https://www.gnu.org/software/make/manual/html_node/Phony-Targets
@technobly

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

@jablko I'm curious what make commands you are running, and what this change helps you achieve. I'm following what you are saying in the problem statement, and I'm also wondering if there is something we might break by making this change.

@m-mcgowan would you please review the make changes you made which were referenced in the past vs. this current proposal?

I currently use only make commands that start with make clean all -s and occasionally make all -s when building just user modules in firmware/main, so I'd be curious to know what other use cases are.

I ran a test between these two sets of commands and see a speed increase using the latter, but no difference between this PR and release/v0.6.1:

// 126 seconds
make clean all -s COMPILE_LTO=n PLATFORM_ID=10 DEBUG_BUILD=y

// 78 seconds
make clean -s PLATFORM=electron
make all -s COMPILE_LTO=n PLATFORM_ID=10 DEBUG_BUILD=y
@jablko

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2017

@jablko I'm curious what make commands you are running, and what this change helps you achieve. I'm following what you are saying in the problem statement, and I'm also wondering if there is something we might break by making this change.

I'm running make program-dfu -- I'm following the getting started guide, here.

I find that it runs the $(TARGET_BASE).elf recipe every time -- even though $(TARGET_BASE).dfu is up to date. Do you see the same thing?

It didn't used to do this -- and it's a mistake for a real target to depend on a phony target:

A phony target should not be a prerequisite of a real target file; if it is, its recipe will be run every time make goes to update that file. https://www.gnu.org/software/make/manual/html_node/Phony-Targets

I don't expect this change will break anything -- it's the same way things worked before ee1d29e -- @m-mcgowan?

@m-mcgowan m-mcgowan added this to the 0.8.0-rc.2 milestone Jan 15, 2018

@m-mcgowan m-mcgowan merged commit 9aa4b8f into particle-iot:develop Jan 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jablko

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2018

Sweet, thank you for merging!

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2018

I did test it and merged, but was clearly not wide enough awake..because program-dfu fails with

dfu-util -d 0x1D50:0x607F -a 0 -s 0x08005000:leave -D newlib_nano
dfu-util 0.8

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2014 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to dfu-util@lists.gnumonks.org

dfu-util: Could not open file newlib_nano for reading: No such file or directory

So we will have to back out the commit until we can fix it. I'm puzzled why this doesn't happen for you, but perhaps changes later in the develop branch have caused it to break?

@jablko

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2018

Oops, I didn't notice that @sergeuz already fixed this. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.