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

build: Add dependency on "all" to top-level make check #970

Merged
merged 2 commits into from Feb 6, 2017

Conversation

Projects
None yet
4 participants
@morrone
Copy link
Contributor

morrone commented Feb 3, 2017

Many of flux-core's tests live down in subdirectories with
the core that that it is testing. However, some of those tests
also have dependencies on other subdirectories higher up the
source tree. With the recursive Makefiles approach, there is
no easy way to express that build dependency in a way that will
actually trigger the build of the that dependency. The following
check-local rule, in conjunction with putting "." first in this
file's SUBDIRS, ensures that "all" is built before any of the
recursive checks.

"check-local" is the only way that was found to add a dependency to
the "check" target. But "make check" is a recursive operation by
default, so the this dependency can only happen first if we also
make "." the first subdirectory in the SUBDIRS list.

Note that this approach will only work at the top level of the
source tree. However, it seems likely that this is not the only
place in the flux-core source tree when a make target needs to be
run at a higher level of the tree in order to be successful. This
is probably an inevitable consequence of the using recursive makefiles
and the design of the ordering dependencies in flux-core.

This was tested to work with "make -j 32 check" as well.

morrone added some commits Feb 3, 2017

build: Add dependency on "all" to top-level make check
Many of flux-core's tests live down in subdirectories with
the core that that it is testing.  However, some of those tests
also have dependencies on other subdirectories higher up the
source tree.  With the recursive Makefiles approach, there is
no easy way to express that build dependency in a way that will
actually trigger the build of the that dependency.  The following
check-local rule, in conjunction with putting "." _first_ in this
file's SUBDIRS, ensures that "all" is built before any of the
recursive checks.

"check-local" is the only way that was found to add a dependency to
the "check" target.  But "make check" is a recursive operation by
default, so the this dependency can only happen first if we also
make "." the first subdirectory in the SUBDIRS list.

Note that this approach will _only_ work at the top level of the
source tree.  However, it seems likely that this is not the only
place in the flux-core source tree when a make target needs to be
run at a higher level of the tree in order to be successful.  This
is probably an inevitable consequence of the using recursive makefiles
and the design of the ordering dependencies in flux-core.

This was tested to work with "make -j 32 check" as well.

Fixes #399
@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 4, 2017

Coverage Status

Coverage decreased (-0.02%) to 76.168% when pulling 3e6fb94 on morrone:issue399 into c44b98b on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 4, 2017

Codecov Report

Merging #970 into master will increase coverage by -0.02%.

@@            Coverage Diff             @@
##           master     #970      +/-   ##
==========================================
- Coverage   75.91%   75.89%   -0.02%     
==========================================
  Files         152      152              
  Lines       25937    25937              
==========================================
- Hits        19689    19685       -4     
- Misses       6248     6252       +4
Impacted Files Coverage Δ
src/common/libflux/keepalive.c 86.66% <ø> (-6.67%)
src/connectors/local/local.c 87.87% <ø> (-1.02%)
src/broker/modservice.c 63.15% <ø> (-0.88%)
src/common/libflux/rpc.c 90.22% <ø> (-0.76%)
src/common/libflux/message.c 83.57% <ø> (-0.27%)
src/common/libflux/handle.c 86.2% <ø> (+0.26%)
src/common/libflux/dispatch.c 82.43% <ø> (+0.56%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c44b98b...3e6fb94. Read the comment docs.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 4, 2017

This has been a long-standing annoyance that nobody has taken the time to fix! Thank you!

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Feb 6, 2017

I don't have any experience with the codecov/project tool. Should I take its errors on this pull request seriously? I would not have expected this pull request to change the things it said changed...not impossible I suppose, but I'm not terribly clear on how that could have happened.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 6, 2017

Should I take its errors on this pull request seriously?

No, I don't think so. Unless coverage drops drastically or in related code then coverage changes are meaningless.

@grondo grondo merged commit 4270ead into flux-framework:master Feb 6, 2017

3 of 4 checks passed

codecov/project 75.89% (-0.02%) compared to c44b98b
Details
codecov/patch Coverage not affected when comparing c44b98b...3e6fb94
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 76.168%
Details

@morrone morrone deleted the morrone:issue399 branch Feb 17, 2017

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

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