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

[KERNEL] Implement kernel release comparison against <www.kernel.org> #94

Merged
merged 4 commits into from
Dec 18, 2020
Merged

[KERNEL] Implement kernel release comparison against <www.kernel.org> #94

merged 4 commits into from
Dec 18, 2020

Conversation

SomethingGeneric
Copy link
Contributor

@SomethingGeneric SomethingGeneric commented Dec 14, 2020

My changes add an entry file which queries kernel.org for the latest stable kernel version.

Description

archey/entries/kernel_latest - entry class
archey/__main__ - added import for kernel_latest and subsequently to the Entries enum
setup.py - I'm lazy and I used python requests (Sorry if I should've used a builtin, that can change?)

Reason and / or context

I saw that kernel.org has metadata for latest kernel, and I think it's neat to see how out of date (or not?) your distribution's kernel is.

How has this been tested ?

Only tested on my Arch machine. However, I can't think of any way it would cause issues.
TODO I don't really know how to write tests, so my pull doesn't include one.
(The only issue would be if the host doesn't have a connection, but I think the other net related ones also fail unsafely?)

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
Copy link
Owner

Hello @SomethingGeneric !

My thoughts about you feature below :

  • Archey is oriented on system information, not on various unspecific ones ;

  • Showing latest kernel release might be interesting though, but not on a dedicated entry IMHO (mainly due to cross-platform constraints in the future, behavior on Windows or macOS ?) ;

  • Why not adding it to the Kernel entry for instance ? User preference ? Disabled by default ? What about non-rolling GNU/Linux distributions ? Would it still be relevant ? (I personally don't think so) ;

  • As an external request is performed, if we keep your implementation, it should honor DO_NOT_TRACK (see 51cdfa9) ;

  • Archey does not fail unsafely on network-related issues ;

  • As you pointed out, builtin module usages (as urllib, see WAN_IP) would be appreciated instead of requests ;

  • I didn't check kernel.org/ API documentation, but if you have, is it rate-limited ? If so, what is the limit ?

Thanks, bye 👋

@HorlogeSkynet HorlogeSkynet added enhancement ⬆️ Implements a new feature, fixes or improves existing ones question ❔ The OP is asking for something about this project ! labels Dec 14, 2020
@HorlogeSkynet HorlogeSkynet self-assigned this Dec 14, 2020
@HorlogeSkynet HorlogeSkynet added this to IN PROGRESS in Core via automation Dec 14, 2020
@HorlogeSkynet HorlogeSkynet added this to the v4.10.0 milestone Dec 14, 2020
@SomethingGeneric
Copy link
Contributor Author

Hi!
I did think about the platform issue, as obviously someone on MacOS, BSD, or Windows doesn't care about the latest linux kernel. So yeah, I think it'd be better as an opt-in thing. And/or, does archey have a method to enable/disable entries based on the host platform? If not, maybe I could implement per-platform disabling?

It didn't occur to me to put it in kernel. Can an entry have more than one item output anyway?

I think it's an extra interesting comparison on non-rolling distros, like how Debian is still on 4.9.0.

And I'll modify to use urllib and to honor DO_NOT_TRACK. I didn't know about that variable.

@SomethingGeneric
Copy link
Contributor Author

I'm sure there's a more elegant way to go about displaying both attributes in the archey/entries/kernel/ output() bit, but I'm not sure what that'd be. Also, either way the kernel test probably has to change.

@HorlogeSkynet HorlogeSkynet changed the base branch from master to develop December 15, 2020 16:23
@HorlogeSkynet
Copy link
Owner

HorlogeSkynet commented Dec 16, 2020

[...], does archey have a method to enable/disable entries based on the host platform? If not, maybe I could implement per-platform disabling?

At the moment, it does not (entries have always work independently in the past), but one day it may.
In this very case (GNU/Linux platforms), sys.platform == 'linux' will be our check.

It didn't occur to me to put it in kernel. Can an entry have more than one item output anyway?

Since the rework done in #70, yes, an entry may now be defined by an arbitrary serializable object.

I think it's an extra interesting comparison on non-rolling distros, like how Debian is still on 4.9.0.

4.19 for Debian 😉
I think we will keep this feature disabled by default once implemented, most of users of stable and frozen distributions actually don't care very much about the latest kernel release.

And I'll modify to use urllib and to honor DO_NOT_TRACK. I didn't know about that variable.

Thanks for urllib. This morning I've added a new Environment singleton for this purpose (see 4425eda), I'll rebase and work on your branch soon.


I'm sure there's a more elegant way to go about displaying both attributes in the archey/entries/kernel/ output() bit, but I'm not sure what that'd be.

[RFC] What do you think about:

# When up to date.
Kernel: X.YY.ZZZ-RR-ARCH (latest)

# When outdated.
Kernel: X.YY.ZZZ-RR-ARCH (T.UU.VVV available)

A new configuration key would be added to Kernel entry type named check_version.
Comparison would be performed on major/minor/patch semver segments.
Of course, I'd leave latest & available strings to configuration for i18n.

For API, Kernel object would be :

{
	"release": "X.YY.ZZZ-RR-ARCH",
	"latest": "T.UU.VVV",
	"is_outdated": false
}

Also, either way the kernel test probably has to change.

I'll deal with tests.


'waiting for your input !
Bye 👋


EDIT : I've ended up implementing my proposal on your (rebased) branch, tell me what you think about that 👌

@SomethingGeneric
Copy link
Contributor Author

SomethingGeneric commented Dec 16, 2020

Hey! I like the style you suggest!

Environment singleton is a clean method for accesssing DO_NOT_TRACK right?

Comparison would also be cool. Perhaps a regex to get rid of the -ARCH bit? Or is that what you did already?

@HorlogeSkynet
Copy link
Owner

Yes, the object will be loaded only once and this way "computations" around os would be DRY-ed for variables altering "global" behavior.

Comparison is already implemented. As we convert the release from a string to Tuple[int] representing each semver segments, a REGEXP is actually not required here.

👋

@SomethingGeneric
Copy link
Contributor Author

Awesome! Is there anything else you'd like me to do to get this closed & ready?

@HorlogeSkynet
Copy link
Owner

Nothing, bar testing this locally if you could (and not already done) 👌
Expect v4.10 to go out soon after this.

@SomethingGeneric
Copy link
Contributor Author

Hey! Sorry for delayed response. Unfortunately, every machine I own running a supported OS is Arch Linux, so my real world tests are essentially useless. (FreeBSD support is in the works ;) )

Anyways, here's it working awesome on my main Arch Machine!
Archey-Demo

@SomethingGeneric
Copy link
Contributor Author

Out of curiosity, I tried it on a Slackware vm I have. Not sure if it's absence is related to my poor Slackware skills or what?
(Maybe because Slack still has major version 4? I'm not sure)
(Also not sure if it matters)

slack

@HorlogeSkynet
Copy link
Owner

HorlogeSkynet commented Dec 18, 2020

Out of curiosity, I tried it on a Slackware vm I have. Not sure if it's absence is related to my poor Slackware skills or what?

Just because check_version is disabled by default, and I assume you enabled it in config on your Arch machine !

You reminded me of the *BSD cases, I've published a patch to only retrieve the latest kernel release on GNU/Linux platforms (to avoid inconsistency between incomparable kernels).

If it's OK for you now, I'm ready to merge this (I've also cleaned up your branch so we won't have to squash the whole thing up).


Completely separated concern about your latest screenshot.

Is your Slackware up-to-date ? I've noticed your lm-sensors package does not provide -j option, so I guess the version packaged/installed is rather old/outdated.
In the past I'd preferred not hiding STDERR messages coming from sensors to let the user know of a potential configuration issue in chipset names.
I don't know how we can manage something for Slackware (if -j is effectively not provided in stable) without implementing a non-JSON parsing...


👋


EDIT : Build is currently failing against Pypy due to actions/setup-python#171.

@HorlogeSkynet HorlogeSkynet changed the title Add entry to display the latest available Linux Kernel [KERNEL] Implement kernel release comparison against <www.kernel.org> Dec 18, 2020
@SomethingGeneric
Copy link
Contributor Author

SomethingGeneric commented Dec 18, 2020

So I did have quite a bunch of outdated stuff, but it's worth mentioning that I did make sure to have a custom config in Slackware, which still didn't yield a kernel comparison.

Also perhaps worth nothing in README that Slack doesn't have Python3 in official repos, perhaps?

Also when you say

You reminded me of the *BSD cases ...

I assume you mean working on future BSD support, right? I tried running it on FreeBSD which is what lead me to notice it didn't work, and that there's an issue for it.

Anyway, in regards to merging this, at least on my main machine, I don't see why not.

@SomethingGeneric
Copy link
Contributor Author

SomethingGeneric commented Dec 18, 2020

So I've updated Slack, and sensors is unhappy still. I think that's just a VM thing, as sensors-detect doesn't find any sensors.
Also, you were right, the only kernel issue was that my config was in /tmp/.config/archey4/config.json (Not sure how I ended up with that CWD but oh well.)

If someone running Slack for real has the same issue with lm_sensors then maybe it's worth looking into.

Anyway, both of my platforms are working fine as far as this pull is concerned! :)

Screenshot from 2020-12-18 09-34-02

@HorlogeSkynet
Copy link
Owner

Also perhaps worth nothing in README that Slack doesn't have Python3 in official repos, perhaps?

Well, I think getting Python installed on its environment is a separate concern from an Archey PoV.

Also when you say

You reminded me of the *BSD cases ...

I assume you mean working on future BSD support, right? I tried running it on FreeBSD which is what lead me to notice it didn't work, and that there's an issue for it.

About that, I meant anticipating the *BSD support (as you pointed out, #69 is on its way) and not breaking more things while it's not there 😮

Anyway, in regards to merging this, at least on my main machine, I don't see why not.

Awesome, let's go then 🙇


So I've updated Slack, and sensors is unhappy still. I think that's just a VM thing, as sensors-detect doesn't find any sensors.

JSON output support landed in lm-sensors v3.5.0, so I guess your packaged version is older than that ?

Also, you were right, the only kernel issue was that my config was in /tmp/.config/archey4/config.json (Not sure how I ended up with that CWD but oh well.)

👌 💯

@HorlogeSkynet HorlogeSkynet merged commit ba3a8da into HorlogeSkynet:develop Dec 18, 2020
Core automation moved this from IN PROGRESS to DONE Dec 18, 2020
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 question ❔ The OP is asking for something about this project !
Projects
Core
  
DONE
Development

Successfully merging this pull request may close these issues.

None yet

2 participants