Skip to content

Fix header file checks to not depend on hardcoded paths#681

Merged
leekillough merged 1 commit into
ROCm:developfrom
leekillough:header-compilation-tests
Sep 5, 2019
Merged

Fix header file checks to not depend on hardcoded paths#681
leekillough merged 1 commit into
ROCm:developfrom
leekillough:header-compilation-tests

Conversation

@leekillough
Copy link
Copy Markdown
Contributor

resolves #664

Change header_compilation_tests.sh to use CMake variables and data extracted from files, instead of hard-coded paths.

I have verified that it works in a /tmp build directory instead of the standard rocBLAS build tree. Running install.sh and running make from build/release still run the header checks.

We are nearing code-complete and this will be merged into master (or the staging branch) shortly, which will add parallelism to the existing script on master, making it less annoying during builds.

@pfultz2
Copy link
Copy Markdown
Contributor

pfultz2 commented Sep 4, 2019

This test should be disabled when using a normal build. Besides adding unnecessary extra build time to rocblas, it also uses hardcoded paths that will break when hcc and hip are in non-standard paths.

The best solution is to create cmake targets that are EXCLUDE_FROM_ALL instead of doing this in bash. In MIGraphX, we generate header tests automatically in cmake here(and the target is created with EXCLUDE_FROM_ALL here) that also includes the header across TUs so we dont accidentally create duplicate symbols by forgetting an inline or static.

Of course, that is more work, so in the short term you can disable the test when tests are disabled in cmake via the BUILD_TESTING variable.

@leekillough
Copy link
Copy Markdown
Contributor Author

If there are more hard coded paths which need to use variables instead, we can add them.

This is intentionally done at build time, because it's closely tied to the build make system, and the rocBLAS test build system is very dissimilar. Also, errors in this script are hard build errors, not just test failures, meaning that the product is unreleaseable.

The time is minimal with parallelism (<10 seconds the first time; instantaneous on later builds). You are basing it on the old version, which was not parallel.

The compiler arguments are hard-coded intentionally to test different use-case scenarios. If one of them fails, it is a concern to us. We want to ensure different compilers and different languages can compile all headers.

@pfultz2
Copy link
Copy Markdown
Contributor

pfultz2 commented Sep 4, 2019

If there are more hard coded paths which need to use variables instead, we can add them.

The cmake targets should be used for the usage requirements not a bunch of variables.

This is intentionally done at build time, because it's closely tied to the build make system, and the rocBLAS test build system is very dissimilar.

As a user installing rocBLAS, it is not necessary.

Also, errors in this script are hard build errors, not just test failures, meaning that the product is unreleaseable.

Yes, but that should've been checked by the CI before merging. As a user, I assume you have already tested all these things, I just need to build and install rocBLAS.

The compiler arguments are hard-coded intentionally to test different use-case scenarios. If one of them fails, it is a concern to us. We want to ensure different compilers and different languages can compile all headers.

You can make the tests for different languages in cmake, and also you can set different compilers in the clients if you want to test different compilers for the same language.

Ideally, you would have a C test in rocblas to test the headers with vanilla gcc. Another test in rocBLAS to test the headers with hip::host and another test with hip::device. Then you can have an additional test in the clients with g++(in fact the clients dont need to be changed since they already include the rocblas headers).

@leekillough
Copy link
Copy Markdown
Contributor Author

As a user, with an unknown ROCm installed, unknown previous version of rocBLAS installed, etc., we need to run these "tests" at build time.

We may add other checks in the future. Two weeks ago, the original issue was taken over by someone who could not build rocBLAS without it hanging, until he removed a previous rocBLAS.

As we transition between different compilers and GPU targets, this kind of checking is best done at build time, to save time later. .githooks is not the place to check it, because Git commands are unrelated to build status -- someone may want to make commits before builds. These checks should be done post-builds, not pre-commit. They verify that rocBLAS is compatible with the user's build environment, while allowing flexibility.

Copy link
Copy Markdown
Contributor

@zaliu zaliu left a comment

Choose a reason for hiding this comment

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

LGTM; thanks so much Paul for your effort and comments. We can continue to make improvements later.

Copy link
Copy Markdown
Contributor

@amcamd amcamd left a comment

Choose a reason for hiding this comment

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

LGTM

@leekillough leekillough merged commit 0277d9e into ROCm:develop Sep 5, 2019
@leekillough leekillough deleted the header-compilation-tests branch December 27, 2019 00:29
mlse-lib-jenkins pushed a commit that referenced this pull request May 12, 2021
Merge rocBLAS PR#1190 to use CMAKE_CXX_COMPILER_ID to detect Clang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rocBLAS no longer builds

4 participants