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

For the buildsystem now only .d files that exist are included #4026

Merged
merged 2 commits into from
May 19, 2020

Conversation

fingolfin
Copy link
Member

Fixes #4025

Of course ideally we'd also have tests for this... Here is one way we could do that: we could have a Travis test which uses an out-of-tree build where we do make clean before make but after configure. That'll trigger the bug as I fixed it.

We could of course do more thing:

  • after the make, remove one file *.d file then run make and verify it is being recreated
  • the same but for an .lo file
  • add in a "bad" .d file which emulates a leftover from a renamed file (with invalid syntax in it) and verify it is not included (as in: make doesn't run into an error)
  • verify that make ; make does not regenerate gap or anything (say, by testing that the second make has no output)
  • test mixing an in-tree and an out-of-tree build to verify the OOT build does not accidentally use files from the in-tree build (say by first doing the in-tree build, then replacing all .d and .lo files with garbage, then doing the OOT build -- if it picks up one of the wrong files, it'll run into an error)
  • ... more can be added over time. That would have caught some of the regressions I accidentally introduced. Of course it won't catch all problems, but we can at least give it a try

@fingolfin fingolfin added regression A bug that only occurs in the branch, not in a release topic: build system labels May 13, 2020
@coveralls
Copy link

coveralls commented May 13, 2020

Coverage Status

Coverage remained the same at 84.481% when pulling 2417390 on fingolfin:mh/fix-buildsys into 0b4c569 on gap-system:master.

@fingolfin
Copy link
Member Author

I added a bunch of tests. They involving using the name gen, so there is an indirect conflict with PR #4013 here. I suggest this PR here is merged first (as I think merging PR #4013 will trigger the issue this PR here is meant to fix for everybody working on GAP!). Once this PR here is merged, I'l update PR #4013.

@@ -290,7 +290,7 @@ endif
DEPFILES = $(patsubst %,gen/deps/%.d,$(SOURCES))

# Include the dependency tracking files, skip any missing ones
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the comment already that we "skip any missing ones", but that wasn't true anymore with my previous refactoring of the buildsystem; this PR restores the behavior described in the comment

@fingolfin fingolfin merged commit 3a9917e into gap-system:master May 19, 2020
@fingolfin fingolfin deleted the mh/fix-buildsys branch May 19, 2020 10:53
@PaulaHaehndel PaulaHaehndel self-assigned this Feb 16, 2021
@PaulaHaehndel PaulaHaehndel added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Feb 16, 2021
@PaulaHaehndel PaulaHaehndel removed their assignment Feb 16, 2021
@PaulaHaehndel PaulaHaehndel changed the title buildsys: only include .d files that exist For the buildsystem now only .d files that exist are included Feb 17, 2021
@fingolfin fingolfin added release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes and removed release notes: added PRs introducing changes that have since been mentioned in the release notes labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression A bug that only occurs in the branch, not in a release release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buildsys: out-of-tree builds regression related to config.h
4 participants