Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for CUDA 10.2 #886
Add support for CUDA 10.2 #886
Changes from all commits
3f5acf3File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this here is not ideal either, since for example package managers in unanticipated environments might run your tests and a simple warning will mark a package as broken.
Please just enable
-Werror=all-warningsin CI, e.g. via a travis environment variable or additional CMake option of Alpaka (that is by default not on). Shipping auto-enabled-Werrorflags of any kind in CMake scripts is a bug :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already did this for years with the normal compilers Werror flag as most other open source projects are also doing. So this is at least not regression.
Furthermore it is the exact goal of setting the Werror flag to let the tests fail if they do not compile cleanly. Why should we treat warnings differently to compile errors? Having no watnings is our definition of our tests passing. So if a package manager for any reason (I do not know why it should do this at all) runs the tests and they produce a warning on this platform, then our tests fail and this is exactly what we want. That's the whole goal behind Werror.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just telling you from a package management point of view, that will use software with newer environments than what were available and testable at release, that this is not an ideal practice.
Package managers such as spack, homebrew, et al. run tests to see if a package is working in a larger environment (integration tests). One usually just runs what comes with the package, to avoid duplication and maintenance overhead.
As developers, we can recommend best practices but it is not our place to anticipate new additions to
-Wall,-Wextraor whatever a compiler decides to warn on. There are more compilers and workflows out there than what we do daily, e.g. people that build their own compiler frontends, optimizer passes, tooling, etc. on projects such as ours. Maybe a new NVCC just warns on something totally minor. Let's not patronize them. Warnings are called warnings for a reason.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. However a newer, self built or unknown compiler is in no way different to newer CUDA versions, newer or self built CMake versions, newer OSs or any other dependency. The Compiler is also only a dependency of alpaka. All of those dependencies can make the build or the tests fail at compile or runtime.
I do not see a reason why we should have lower requirements than is possible on the compilation while we expect everything to generate, run and calculate perfectly.
If we lower the requirements on one dependency, we should also reduce the requirements on numerical stability, quality of random numbers and other things that may behave unexpectedly on untested platforms.
Finding issues on untested platforms is the goal of tests and for alpaka a clean compilation is what we defined as a requirement for a successful test execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not that black and white. A compiler warning in a newer nvcc might just be fine, no reason to break the tests outside of CI for this. A test that passes and throws a warning is still a tests that passes its primary coded purpose. A CMake flag that also enables
-Werrorintentionally can then also be used on top of this.This is just compartmentalization of goals into smaller scopes of interest. Our goal in mainline is to have the code base error and warning free. That's why we require
-Werrorfor all code to come in. A user's primary downstream goal is to have the tests technically compute the right results as a baseline for whatever they want to do. They can decide if they want to treat warnings as errors as well. I tell you from experience that many use cases do not want this to be the case, because warnings are not errors, some warnings are false positives in new environments, etc. Let's just optionalize this and enable this for us.