lpereira / hardinfo

System profiler and benchmark tool for Linux systems
http://hardinfo.org
GNU General Public License v2.0
765 stars 130 forks source link

Version 1.0.1 pull request #708

Closed hwspeedy closed 8 months ago

hwspeedy commented 8 months ago

Changed license to GPL2+ - see issue #707 and more. Clean up documentation for users and distros. GTK3 - GTK2+ build still possible use: cmake -DHARDINFO_GTK3=0 .. added EXPERIMENTAL LibSoup 3.0 minor crash fixes, compiler compatibility and updates. added long overdue PR Create packager and tested it for most used distro - see v1.0.1pre attachments.

mckaygerhard commented 8 months ago

hi @lpereira I have been super absent because like yours we evolves to other ways or superation... of course I don't work for M$ due my moral and ideals, but here we go...

Something interesting is that the benchmark results in GTK2+ are sometimes much better than in GTK3+, demonstrating that in minimalist distros as well as in options where performance is desired... using GTK2 should not be left aside.

i mean the benchmarks are not the same in GTK3 agains GTK2 flavours.. so there are real benchmarks?

The differences are minimal but can only be seen well on less powerful devices or machines, by example linux runing on phones (a only allowed solution for most people) or cheap/older computers still working and around hole real world.

hwspeedy commented 8 months ago

Hi @mckaygerhard,

Thanx for feedback on PR, much appreciated.

It is no problem to keep GTK2+ support - it is very alike to GTK3. GTK4 is another ball game :)

To build with GTK2+: cmake -DHARDINFO_GTK3=0 ..

Tested - found bug and fix submitted in this PR.

Regarding benchmarks - they are the same - the GPU benchmark was disabled/hidden long ago. GTK2 vs GTK3 is just UI and keeping up with distros.

mckaygerhard commented 8 months ago

To build with GTK2+: cmake -DHARDINFO_GTK3=0 ..

Tested - found bug and fix submitted in this PR.

err yeah ok so fast i was about to submit an issue but i see that you @hwspeedy took some time to corroborate the lack of attention... although i noticed that it is forcing some modern dependencies at least now compiles with gtk2, much appreciated your support on

about the rest you should use real hardware and very older computers.. i can upload telegram videos some time later cos i am pretty busy as a works slave :face_with_spiral_eyes:

mckaygerhard commented 8 months ago

@hwspeedy and @lpereira about domain, provide a default file at /etc/hardifo that "if present" will point to the domain, and if not just use a hardcoded default..

lpereira commented 8 months ago

@hwspeedy and @lpereira about domain, provide a default file at /etc/hardifo that "if present" will point to the domain, and if not just use a hardcoded default..

There's no need to have a setting for the domain. That's the reason api.hardinfo.org has been established.

lpereira commented 8 months ago

hi @lpereira I have been super absent because like yours we evolves to other ways or superation... of course I don't work for M$ due my moral and ideals, but here we go...

I don't know what working for Microsoft has anything to do with HardInfo, but I don't work for them anymore. In any case, GTK+2 will be supported for the next version, but there's a good chance that, if HardInfo moves forward as a project, it'll eventually be dropped in favor of something newer. There are many things in newer versions of GTK that I'd like to use but would be quite a bit difficult to maintain two versions of the shell.

lpereira commented 8 months ago

@hwspeedy There are over 120 commits in this PR. It's pretty hard to review as it is. I'm going to ask to please break down this into smaller PRs, as I don't have enough free time or the energy to go through large PRs, especially if it keeps growing every day.

Also, why 1.0.1? 1.0 wasn't even released, there's no need to have a point release.

hwspeedy commented 8 months ago

@lpereira Yes, I know it is not the best way, but I had to get momentum to get forward on this project, sorry.

The problem with dividing commits up is that I made this mistake with license change - where I thought that it was enough that you said okay for all the files that you have copyright on. That is then fixed later along with work on maintenance fixing. Therefor it cannot be divided up, sorry.

I am done submitting for now - it is a pretty good app now!

But I will need you to go through this 121 commits PR.

  1. If there is any fixing error, that will be my problem - but I have done extensive testing. And you can check each fixing submit - but don't have to.
  2. The licenses changes you can see in files mode and check, that where I have changed to GPL2 or Later - the copyrights holders have said OK.
  3. The documentation updates you can see in files mode.

mess 0.6, 0.7 are in the wild as no release was made for >10 years. 1.0.0 first working GTK3 prerelease 1.0.1 prereleased tested on lots of systems 1.0.2 will be next prerelease - the first that the project will actually see and can comment on. Really looking forward to that. 1.1.0 will be release

The scheme will be that when released the number is stepped to 1.1.1 so that peoples/developers HEAD version can coexists with distro releases. If developers put in alot of changes or larger portions the number is again stepped to 1.1.2. Then some prereleases 1.1.17 and maybe 1.1.18. The future release is then 1.2.0.

Is that wrong versioning for releasing?

mckaygerhard commented 8 months ago

ok most of my reviews are only due minimal things but this is pretty not for me https://github.com/lpereira/hardinfo/pull/708/files#diff-2da0b72ef7844ae5b1d9a5a5fb3915fada4ddb718c2e5b76de01f44462f9a52aR449

the call to memset() clear hasn't any externally visible effect according to the C language specification, so it can be removed by the optimize

the rest @hwspeedy loks good to me! just the changes of links are ilogical if you pretent to merge this pull so change it!

lpereira commented 8 months ago

the call to memset() clear hasn't any externally visible effect according to the C language specification, so it can be removed by the optimize

This is not only false, it's wrong. The call to memset() there is needed to zero-initialize those structs, that aren't initialized with {} anymore according to the previous hunk in the diff.

lpereira commented 8 months ago

The problem with dividing commits up is that I made this mistake with license change - where I thought that it was enough that you said okay for all the files that you have copyright on. That is then fixed later along with work on maintenance fixing. Therefor it cannot be divided up, sorry.

You can squash the commits -- I often review commit by commit, so if one commit has one change and then there are a few other commits undoing changes or fixing things in the previous commits, it's just wasted time. I know this might take more time, but reviewing takes more time than writing code, and my time is incredibly limited these days.

mckaygerhard commented 8 months ago

the call to memset() clear hasn't any externally visible effect according to the C language specification, so it can be removed by the optimize

This is not only false, it's wrong. The call to memset() there is needed to zero-initialize those structs, that aren't initialized with {} anymore according to the previous hunk in the diff.

umm but in optimization levels seems this instructions are removed if the previous value are null so you are right only cos the code assumes that in those lines always the values will be initialized by garbage inclusivelly but at logic level.. .. ok of course i am not xpert but i m pretty sure that only works for non optimized builds. nevermind.. so leave it if there is no dangerous consecuences

about the review.. maybe we ned more people if you dont have time @lpereira !

hwspeedy commented 8 months ago

@mckaygerhard "the rest @hwspeedy loks good to me! just the changes of links are ilogical if you pretent to merge this pull so change it!" "about the review.. maybe we ned more people if you dont have time @lpereira !"

The links are changed for matching the repo transfer to hwspeedy. I will do maintaining as @lpereira does not have the time and has moved on. Maybe she will be back as a developer when the release is done. It has been mainly a one person job from the look at the git log with some heavy lifting from 3 contributers - That will be changed - we need a lot more people to be interested in development of the project hardinfo. The PR will be handled much faster and the community will help check every commit and send feedback. I will ofcourse look them through and also test them on the new setup with 10+ different machines all sending the results to my PC for testing. - Just finished today along with some backporting (GTK2+ on ol6) and fix of "benchmarking..." dialog + more.

Please @lpereira - It will all be good and we will all get this project running again. Please show trust - not alot of projects gets a new maintainer these days. Can we get the full change into the repository so they are in sync and get the repo transfer done?

I need to submit more fixes and make a new prerelease. :)

Br

Hwspeedy.

mckaygerhard commented 8 months ago

umm all of this makes so much sense..

The links are changed for matching the repo transfer to hwspeedy. I will do maintaining as @lpereira does not have the time and has moved on Maybe she will be back as a developer when the release is done. It has been mainly a one person job from the look at the git log with some heavy lifting from 3 contributers - That will be changed - we need a lot more people to be interested in development of the project hardinfo. The PR will be handled much faster and the community will help check every commit and send feedback. I will ofcourse look them through and also test them on the new setup with 10+ different machines all sending the results to my PC for testing. - Just finished today along with some backporting (GTK2+ on ol6) and fix of "benchmarking..." dialog + more.

ok so then @hwspeedy has a valid point! leaving the part of the links and the memctp with the changes i proposed all looks good to me! but i guess event a personal repo must be an organization @hwspeedy !

hwspeedy commented 8 months ago

@mckaygerhard "but i guess event a personal repo must be an organization @hwspeedy"

I completely agree - I have added you as collaborater on the branch - You have already shown willingness to do reviewing work. After transfer completed, I will add you to organisation.

It is all about developing together.

hwspeedy commented 8 months ago

Hi lpereira,

All the FIX commits are made correctly as you describe - it is especially the README.md that will help reduce number of commits and it is a good file to do trials with.

The internet says I should not do force and squash when a PR is for review, but here we go:

I tried to force push the README.md but it did not work on my github history at my branch:

  1. change README.md a little
  2. git commit -a
  3. git push origin master --force Then I expected the commit history to be a lot smaller - but it grew with another commit.

What am I doing wrong?

I have allowed squashing from branch in my repo - that will of course squash all commits but it is safe - can you do the same squashing when merging branch?

Otherwise I have to do the safe way I know and use - export changes as patches and create new fork.

Br

hwspeedy

lpereira commented 8 months ago

Don't force-push to main. Create a new branch; if you can't convince GitHub to use the new branch in the PR, feel free to close this one and open another. Once you have a PR set up with a branch that's not main, you can force-push to it just fine. But you'll probably not need to force-push if you have the history just right before you do it. Maybe sending multiple PRs, one for each set of changes you do, may be actually a better idea.

I'm not suggesting you squash all commits -- I'm suggesting you squash/fixup commits that pretty much do the same change (say, you change something in commit 1, then in commit 20 you change it to something else because you messed up in commit 1, the history doesn't need to show that: you squash 1 and 20 together). You can do this with an interactive rebase, you can find tutorials on how to do it online.

Another thing that's important: all the commit messages for the commits changing licenses must contain some text saying that the license change has been acknowledged by the copyright holder, with a link to their respective comments. You have to be really careful when it comes to software licensing, and I wasn't when I merged the PR that added GPL3 code to the codebase, so there's now a mess to clean.

Since you've never contributed to the project before, I'd like to understand you better before anything in the direction of you becoming the new maintainer happens. Maybe we should schedule a video call to discuss?

hwspeedy commented 8 months ago

Sounds very good with a video meeting.

I am on skype:speedy19742 or discord:hwspeedy#6613

When will be a good time for you? - we have some time difference to USA,WA - Denmark - it is 6am here and 9pm at your place. +15 hours.

mckaygerhard commented 8 months ago

@hwspeedy you can use cherry-pick command with brach git remote mix for.. this will easy your new PR.. (maybe this info is just trivial but who knows)

hwspeedy commented 8 months ago

@mckaygerhard Thanx, I tried - but it failed due to mixing of code and lots of documentation commits.

I have recreated the local repo and have all the patches worked out. Now it looks as if I had the complete situation overview from the start of commiting :)

@lpereira is going to call me for video meeting.

I have created a minimum PR with GTK3 FIX, LICENSE change, MAINTAINER change, Packaging and workflow scripts - done to respect that lpereira has very limited time.

https://github.com/lpereira/hardinfo/pull/710