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

Closes Issue333 bisect extraneous runs #337

Merged
merged 8 commits into from
Jun 15, 2021

Conversation

JohnJacobsonIII
Copy link
Collaborator

Fixes #

Description:

Fix for bug that caused ground truth executable to be re-tested on every make call.

Bug originated from labeling executables as PHONY in Makefile.in; as a result all targets depending on the executable run every time make is called

Per @mikebentley15, a simple hack adding a layer of separation fixed the issue. The executable is no longer marked PHONY, but is a target with a single-use PHONY prerequisite and a no-op recipe. This still forces recursive calls to build every executable, but doesn't force dependents to run. The no-op recipe also forces Make to verify the timestamp on the executable.

Documentation:

No functionality changed, so no need for documentation changes.

Tests:

  • Added unit test for ground truth re-test specifically. This should probably be expanded to test for more duplication (duplication of any test call, build stage, or anything else...)
  • Required update to existing tst_incremental_build.py. Because of the no-op recipe, the executable is touched on every call; however, Make successfully determines that it doesn't need to be built, so this is simply a test methodology issue. The current fix is a hack, but the test needs to be rewritten to properly test makefile (by actually checking modification times.) I'll create an issue for this.

@JohnJacobsonIII JohnJacobsonIII added bug python Involves touching python code make Involves touching GNU Makefiles tests Involves touching tests bisect Pertains to FLiT Bisect labels Jun 11, 2021
Copy link
Collaborator

@mikebentley15 mikebentley15 left a comment

Choose a reason for hiding this comment

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

I'm satisfied with the fix for the bug being addressed here.

I believe the incremental build test could be changed to use one of the fake compilers, fake_gcc4.py or fake_clang34.py, instead of using make --touch which assumes that every recipe will update its target (and therefore touches it). These fake compilers also touch the output file, but only when the compiler is actually called (which the introduced no-op operation does not do).

I'm fine if that change is part of a different issue. If so, I want the issue number here commented in this pull request.

tests/flit_makefile/tst_incremental_build.py Show resolved Hide resolved
tests/flit_makefile/tst_incremental_build.py Show resolved Hide resolved
@JohnJacobsonIII
Copy link
Collaborator Author

Issue #338 opened to fix tst_incremental_build.py. Temporary hack here does not effectively test the incremental build, but the change here should not affect compilation so I've moved the fix to a separate issue.

@mikebentley15 mikebentley15 merged commit 357696e into devel Jun 15, 2021
@JohnJacobsonIII JohnJacobsonIII deleted the issue333-bisect-extraneous-runs branch June 16, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bisect Pertains to FLiT Bisect bug make Involves touching GNU Makefiles python Involves touching python code tests Involves touching tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants