LinuxCNC / linuxcnc

LinuxCNC controls CNC machines. It can drive milling machines, lathes, 3d printers, laser cutters, plasma cutters, robot arms, hexapods, and more.
http://linuxcnc.org/
GNU General Public License v2.0
1.81k stars 1.16k forks source link

Be more tolerant to string in VERSION file #2778

Closed dwrobel closed 11 months ago

dwrobel commented 11 months ago

When the string in VERSION file contains date and git sha like the following: 2.10.0-0.96.20231128git2217582.fc38.src.rpm

then qtplasma refuses to work as it is unnable to properly find 'major' and 'minor' components from the version string.

So let's parse the string from left where we always expect 'major' and 'minor' to be located.

dwrobel commented 11 months ago

With this change the title bar of qtplasmac will also contain full string from the VERSION file: Screenshot from 2023-12-09 20-07-24

snowgoer540 commented 11 months ago

With this change the title bar of qtplasmac will also contain full string from the VERSION file: Screenshot from 2023-12-09 20-07-24

I agree with the conditional checks for lines 330 and 333.

I am not sure I agree with the added version information at line 133...

Is the specific build information necessary for the package builds? It seems redundant especially for RIP versions. The overall goal here was to provide high level version information without cluttering the title bar, with the major and minor LCNC version being baked into the QtPlasmaC versioning.

If this every becomes necessary for debugging on the forum, etc, it's provided in the config_info.txt file as part of making a backup from the SETTINGS tab.

snowgoer540 commented 11 months ago

Also, the conditional check affects 2.9, and should be done before the 2.9.2 release if it breaks the works.

It seems like it might be better to propose this change to the 2.9 branch, and then we can merge it into master?

dwrobel commented 11 months ago

I am not sure I agree with the added version information at line 133...

I have a feeling the linuxcnc version was always in the title bar:

$ git blame ca2569ae8d6d35e1769b2c42c23f7db26874a276 share/qtvcp/screens/qtplasmac/qtplasmac_handler.py | grep WindowTitle | head -n1
80bba07bc22 (Greg Carl       2022-07-25 09:58:00 -0400  402)         self.w.setWindowTitle('{} - QtPlasmaC v{}, powered by QtVCP on LinuxCNC v{}'.format(self.machineName, VERSION, linuxcnc.version.split(':')[0]))

please also have a look at the screenshots attached to https://github.com/LinuxCNC/linuxcnc/issues/1486 (a year old now) where you could see that the version of the linuxcnc was on the title bar.

It seems redundant especially for RIP versions

I agree, but it's a specific use case. If you're using a software from a .deb or .rpm package then without having an About menu entry it might be difficult to realize which particular version you're using.

For completeness, here is how it looks on the Axis GUI: Screenshot from 2023-12-09 20-06-33

Summing up, if you still prefer not to clutter the title bar with the linuxcnc version I can remove it from this PR.

petterreinholdtsen commented 11 months ago

[Damian Wrobel]

I agree, but it's a specific use case. If you're using a software from a .deb or .rpm package then without having an About menu entry it might be difficult to realize which particular version you're using.

A simple way to figure that out is to ask dpkg or rpm for the version information of the installed package. Not sure the title or about entry make it much easier.

-- Happy hacking Petter Reinholdtsen

snowgoer540 commented 11 months ago

Summing up, if you still prefer not to clutter the title bar with the linuxcnc version I can remove it from this PR.

Yes please; we recently redid the versioning as the version in Master now differs from the 2.9 version, along with that, we cleaned up the title bar as well. :)

This change is also still against the master branch. As I mentioned, this change should really go against 2.9 since it prevents a package version of qtplasmac from running.

Then I can merge/cherry pick this into master.

dwrobel commented 11 months ago

Yes please;

Done.

Then I can merge/cherry pick this into master.

I'm working with master branch only. Is there any problem for you to cherry-pick it to any other branch, if needed.

snowgoer540 commented 11 months ago

Is there any problem for you to cherry-pick it to any other branch, if needed.

I asked Andy, I will let you know what he says.

snowgoer540 commented 11 months ago

Can I get you to version this commit please? Then I will commit it.

I realized this evening that along with this change, hansu's from the other commit, and another small discovery of extra ":" in some dialog boxes pertaining to material creation will all apply to 2.9 as well. Rather than make it complicated, I am just going to make one big commit against 2.9 to take care of all of it, and give you and hansu credit in the comment. Unless you take issue with that?

dwrobel commented 11 months ago

Can I get you to version this commit please?

Done.