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

Make sure build is completed before make test starts tests #22943

Closed
DDvO opened this issue Dec 4, 2023 · 9 comments
Closed

Make sure build is completed before make test starts tests #22943

DDvO opened this issue Dec 4, 2023 · 9 comments
Labels
issue: bug report The issue was opened to report a bug

Comments

@DDvO
Copy link
Contributor

DDvO commented Dec 4, 2023

Copying @hlandau 's description (with editorial adaptations) from anther issue:

When developing I always find running make test after changing a file to result in trying to run some test binaries
before they are actually finished compiling, resulting in an error, which is why I always run make && make test.
We should fix that.

@DDvO DDvO added the issue: bug report The issue was opened to report a bug label Dec 4, 2023
@levitte
Copy link
Member

levitte commented Dec 4, 2023

If you do just make test, I don't see how that could happen. However, if you somehow had -j{n} (explicit or implied somehow), then you're outta luck, because of the parallel dependency where the last one is run-tests:

##@ Testing
test: tests ## Run tests (alias of "tests")
{- dependmagic('tests', 'Run tests'); -}: build_programs_nodep build_modules_nodep link-utils run_tests
run_tests:

@DDvO
Copy link
Contributor Author

DDvO commented Dec 4, 2023

Yep, this must bei the point: parallel make goes wrong then.

@rwfranks
Copy link

rwfranks commented Dec 4, 2023

The real issue here lies in the declared dependency relationships.

##@ Testing
test: tests ## Run tests (alias of "tests")
tests: build_generated ## Run tests
        $(MAKE) depend && $(MAKE) _tests
_tests: build_programs_nodep build_modules_nodep link-utils run_tests
run_tests:

The way things stand, from make's pov "build_programs_nodep", "build_modules_nodep", "link-utils" and "run_tests" are declared to be mutually order independent (i.e. potentially parallel jobs). Neither do I believe that make offers any guarantee to resolve target lists left to right, so in theory even make -j1 could fail.
The build stuff needs to be grouped under a separate target and the tests depend on that target.

@slontis
Copy link
Member

slontis commented Dec 5, 2023

Recently my 'make test' stopped working.. I am not sure what I have done.. (listing the tests shows nothing also)
Adding the following manually to the Makefile addresses the issue..
.PHONY: run_tests

@levitte
Copy link
Member

levitte commented Dec 5, 2023

I hope I fixed it. Please have a look at the PRs

@rwfranks
Copy link

rwfranks commented Dec 5, 2023

PRs look ok.

@levitte
Copy link
Member

levitte commented Dec 7, 2023

Reopening, 'cause #22948 hasn't been merged yet

openssl-machine pushed a commit that referenced this issue Dec 7, 2023
Fixes #22943

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #22947)

(cherry picked from commit f882753)
openssl-machine pushed a commit that referenced this issue Dec 8, 2023
Fixes #22943

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #22948)
openssl-machine pushed a commit that referenced this issue Dec 8, 2023
Fixes #22943

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #22948)

(cherry picked from commit ab2b19e)
@jamuir
Copy link
Member

jamuir commented Dec 16, 2023

@levitte : can this be closed now?

@DDvO
Copy link
Contributor Author

DDvO commented Dec 16, 2023

Closing as this has been merged to 3.0, 3.1, 3.2, and master.

@DDvO DDvO closed this as completed Dec 16, 2023
wbeck10 pushed a commit to wbeck10/openssl that referenced this issue Jan 8, 2024
Fixes openssl#22943

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from openssl#22947)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug report The issue was opened to report a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants