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

[CPU] [FEATURE/BREAKING] Multiple CPUs support #90

Merged
merged 5 commits into from
Oct 24, 2020

Conversation

HorlogeSkynet
Copy link
Owner

@HorlogeSkynet HorlogeSkynet commented Oct 17, 2020

Description

This is an API-breaking change.
Now multiple CPUs should be supported, and number of cores per CPU will be shown by default.

Below examples show output changes for a dual CPUs setup (respectively with N & M cores).

Text output change :

- CPU: A-CPU-MODEL-NAME_1
+ CPU: N x A-CPU-MODEL-NAME_1, M x A-CPU-MODEL-NAME_2

JSON API change :

- "CPU": "A-CPU-MODEL-NAME_1",
+ "CPU": [
+     {"A-CPU-MODEL-NAME_1": N},
+     {"A-CPU-MODEL-NAME_2": M}
+ ],

See also new one_line & show_cores options available for CPU entry type.

Reason and / or context

It appears Archey does not support multiple CPUs (only the first one would be displayed).
Additionally, adding the number of cores may be relevant.

See also dylanaraps/neofetch#1467 partially related.
(@Nalorokk's output suggestion has been kindly implemented there.)

How has this been tested ?

Locally and unit tests.

Types of changes :

  • Bug fix (non-breaking change which fixes an issue)
  • Typo / style fix (non-breaking change which improves readability)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist :

  • [IF NEEDED] I have updated the README.md file accordingly ;
  • [IF NEEDED] I have updated the test cases (which pass) accordingly ;
  • [IF BREAKING] This pull request targets the develop branch ;
  • My changes looks good ;
  • I agree that my code may be modified in the future ;
  • My code follows the code style of this project (PEP8).

@HorlogeSkynet HorlogeSkynet self-assigned this Oct 17, 2020
@HorlogeSkynet HorlogeSkynet added the enhancement ⬆️ Implements a new feature, fixes or improves existing ones label Oct 17, 2020
@HorlogeSkynet HorlogeSkynet added this to IN PROGRESS in Core via automation Oct 17, 2020
@HorlogeSkynet HorlogeSkynet added this to the v4.9.0 milestone Oct 17, 2020
@Nalorokk
Copy link

Seems like detection may be broken in some cases

https://gist.github.com/Nalorokk/91f4cced6f61ad72b6d37595c2662256

@HorlogeSkynet
Copy link
Owner Author

HorlogeSkynet commented Oct 18, 2020

@Nalorokk Oh you actually tried this out 😅
I've thought about your case since this PR is opened, and yeah, when all different CPUs have the same name, cores are just being added (imitating Neofetch current behavior I guess then).
Would you mind sending me your /proc/cpuinfo content (as long as lscpu output if you got it installed) so I may work on this (I actually don't have such a setup available) ?

Bye, thanks 👋

@Nalorokk
Copy link

@HorlogeSkynet here it is https://gist.github.com/Nalorokk/5ce83329610bcc5102639230428014c6 . There is two examples from different machines

@HorlogeSkynet
Copy link
Owner Author

HorlogeSkynet commented Oct 18, 2020

Thank you very much for this.
I've come up with an algorithm, which would give you below final Python objects (if I manage to implement it) :

# For your 1st machine :
[
	{'Intel(R) Xeon(R) CPU E5450 @ 3.00GHz': 4},
	{'Intel(R) Xeon(R) CPU E5450 @ 3.00GHz': 4}
]

# For your second machine :
[
	{'Intel(R) Xeon(R) CPU E5620 @ 2.40GHz': 8},
	{'Intel(R) Xeon(R) CPU E5620 @ 2.40GHz': 8}
]

If you're OK with the idea, I'll update this PR with a new proposal/implementation.

@HorlogeSkynet HorlogeSkynet marked this pull request as draft October 18, 2020 09:13
> Now compatible against Python < 3.6
> Caution : new proposal so still API-breaking
@HorlogeSkynet
Copy link
Owner Author

Hey @Nalorokk, please find on this branch a more robust implementation that should support many setups.
Bye, waiting for your feedback before merging this 👋

@HorlogeSkynet HorlogeSkynet marked this pull request as ready for review October 24, 2020 09:22
@Nalorokk
Copy link

Hello @HorlogeSkynet . I tried new branch and got such output

#########.   .########`      CPU: 4 x Intel(R) Xeon(R) CPU E5450 @ 3.00GHz, 4 x Intel(R) Xeon(R) CPU E5450 @ 3.00GHz

It is way better than before. Maybe line is kinda too long though. But I just thought about it a little more and there is some another problems:

  1. There is quad cpu motherboards
  2. Some of them may work with just 3 cpus installed.
  3. Some motherboards might work with different cpu's mixed.

I don't have quad cpu setups around, but maybe will try mixing different cpus in one motherboard, it happens that i have combo that might work together

@HorlogeSkynet
Copy link
Owner Author

It is way better than before. Maybe line is kinda too long though.

Yes, there is a new option called one_line, for CPU entry. If you disable it, each entry will be on their respective line.
Maybe this option should be disabled be default ? What do you think about that ?

But I just thought about it a little more and there is some another problems:

1. There is quad cpu motherboards
2. Some of them may work with just 3 cpus installed.
3. Some motherboards might work with different cpu's mixed.

Latest implementation should correctly deal with such setups.


Thank you very much again for your feedback @Nalorokk. 👋

@Nalorokk
Copy link

@HorlogeSkynet i think it would be better with one line disabled by default. Thank you for your work on this topic 👍

@HorlogeSkynet
Copy link
Owner Author

i think it would be better with one line disabled by default.

Fair enough, I've just got it disabled by default for CPU (as long as GPU, for configuration consistency purposes).

Thank you for your work on this topic 👍

Sure, thank you for your fast responses and your time @Nalorokk.
Cheers 🍻

@HorlogeSkynet HorlogeSkynet merged commit 87ace15 into develop Oct 24, 2020
Core automation moved this from IN PROGRESS to DONE Oct 24, 2020
@HorlogeSkynet HorlogeSkynet deleted the feature/multiple_cpus branch October 24, 2020 15:01
HorlogeSkynet added a commit that referenced this pull request Nov 22, 2020
* [API] : `CPU` will now be a `list` of `dict[str, int]`, representing CPUs model names and their respective number of cores ;
* [CPU/GPU] : Entries items will now be displayed on multiple lines **by default**.

> See also Neofetch's issue #1467.
> Thanks to @Nalorokk for his suggestions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ⬆️ Implements a new feature, fixes or improves existing ones
Projects
Core
  
DONE
Development

Successfully merging this pull request may close these issues.

None yet

2 participants