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

[vcpkg] [cuda] minor fixes for cuda and vcpkg #11341

Closed
wants to merge 3 commits into from

Conversation

simogasp
Copy link
Contributor

@simogasp simogasp commented May 13, 2020

Describe the pull request

  • What does your PR fix?
  • it fixes the initialization order of some class attributes for vcpkg, clang raise an error (and ISO C++ requires field designators to be specified in declaration order) done in [vcpkg] Turn on tests in CI. #11239

  • minor improvement in cuda port, extract the version directly from the nvcc pattern VX.Y.Z and compare to declared variable CUDA_REQUIRED_VERSION instead of performing a check with hardcoded values. This improves maintainability, next time the required version is changed, it has to be changed in only one place. see [cuda/cudnn] upgrade to 10.2 #11322

  • Which triplets are supported/not supported? Have you updated the CI baseline?
    No changes in terms of supported triplets

  • Does your PR follow the maintainer guide?
    Yes

extract the full version from the pattern VX.Y.Z and compare to
CUDA_REQUIRED_VERSION
@simogasp simogasp changed the title minor fixes for cuda and vcpkg [vcpkg] [cuda] minor fixes for cuda and vcpkg May 13, 2020
@LilyWangL
Copy link
Contributor

Thanks for your PR! Can you please correct the Version field in the cuda CONTROL file? You can get more information from maintainer-guide.md.

@simogasp
Copy link
Contributor Author

@LilyWangL oops my bad, done in 021cf61
(and sorry if I mixed 2 different topics in a single PR, but they are very minor fixes so I figured that it was not worth it)

@cenit
Copy link
Contributor

cenit commented May 14, 2020

Similar to something already present in #11322 (excluding the upgrade there) for the CUDA part?

@simogasp
Copy link
Contributor Author

yes, but IMHO it's better to have a single variable CUDA_REQUIRED_VERSION rather than CUDA_MAJOR etc.
If you agree you can apply the small changes of this PR to yours and I'll drop mines here.

@simogasp
Copy link
Contributor Author

(and thanks for pointing it out, I should have checked open PRs before)

@cenit
Copy link
Contributor

cenit commented May 14, 2020

You can apply a review in the changes tab of my pr, and I will gladly accept it. Otherwise I can do it later

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@simogasp
Copy link
Contributor Author

I'm closing this pr because one contribution is already merged in #11239 and the other has been moved into #11322

@simogasp simogasp closed this May 14, 2020
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.

4 participants