-
Notifications
You must be signed in to change notification settings - Fork 931
configury: minor updates to config summary output #1447
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
configury: minor updates to config summary output #1447
Conversation
|
@sjeaugey Do you want to add something in here about CUDA? |
|
Looks good to me. |
|
You might want to add in opal_check_cuda.m4 before the |
|
CUDA could be in a separate "Others" section because it applies to many parts of the code. But that would make a bigger summary and it is already big, so I think I'm OK with George's suggestion. Other than the CUDA part, I would have a few comments on the original patch :
|
5b42519 to
d5919d7
Compare
|
Ok, good points:
|
|
If space is a constraint why don't we list only what is on ? |
|
It's a conundrum:
Shrug. I don't know what the right answer is. |
|
I personally like projects where I'm also shown what is not compiled, so that I know what I could have. I've seen interesting outputs showing two lines per category, with one line "enabled" and one line "disabled". E.g. : |
|
Fair enough. That's a big revamp compared to the current code, and I can't really justify spending the time to do that (doing these minor tweaks/updates was fine). 😄 If you want to change the output to do the enabled/disabled lines line you suggest, feel free to submit a PR... 😃 |
|
I know. Let's keep it that way for now and improve it when it is needed. I'll be happy to submit a PR at that time. Regarding CUDA, I'm good with George suggested modification. Do you need a patch or a PR ? |
|
Oh, I missed the @bosilca suggestion about CUDA. Let me add something along those lines... I actually think it should be in a separate section -- CUDA is not a transport. |
d5919d7 to
92212d4
Compare
|
Ok, CUDA support is now in there: |
|
@rhc54 Should: be ? |
|
👍 |
1 similar comment
|
👍 |
92212d4 to
48c650c
Compare
|
@rhc54 Confirmed in IM: yes, it should be Intel TrueScale. Fix pushed -- waiting for CI before merging... |
configury: minor updates to config summary output
|
Catching a little late, re-confirming a yes for Intel TrueScale |
This is an addendum to Nathan's original commit (d2f5fca).
Output now looks like this:
@hjelmn Opinions?