Page MenuHomeSolus

Initial commit of GNU poke tui
Needs ReviewPublic

Authored by eradot4027 on Fri, Jun 11, 1:25 AM.

Details

Reviewers
None
Group Reviewers
Triage Team
Summary

Intial commit of GNU poke tui

Test Plan

Tested on Solus Budgie while reviewing and binary file

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 712
Build 712: arc lint + arc unit

Event Timeline

eradot4027 created this revision.Fri, Jun 11, 1:25 AM
eradot4027 requested review of this revision.Fri, Jun 11, 1:25 AM

I downloaded your diff locally and built it so as to give you better feedback than just reading over your diff.

The first thing I'd do if I were you would be to edit your diff summary to say something like:

Closes T9618

This will setup the linking in Phabricator so that it's easy for maintainers to see what package request you're fulfilling with this diff (I think phab is also setup to close the linked issue when this diff is merged).

When I built this package I noticed the following two warnings at the end of the compile phase (there was an additional one about the GUI but that's because you disabled it).

configure: WARNING: libtextstyle was not found in the system. Poke's output won't be styled.
configure: WARNING: building poke without NBD io space support.
     Install libnbd to use it.

The first one is just because the version of gettext in the repos is a version behind (it needs gettext 0.21 while we have 0.20.2). Creating multiple diffs that depend on each other is possible but it's a bit more complicated to setup locally so I'd just ignore it for you (feel free to give it a try if you want, if not I'll probably update gettext after this diff lands and then do a second revision of this package).

The second one is only fixable if we add libnbd to the repos. As far as I can tell that just enables some optional nbd functionality that doesn't look like it would be widely used so I'd probably just ignore this one as well.

package.yml
14–15

You can actually drop both of these packages. They're part of system.base and are thus always installed in the build environment.

17

I believe you can remove this FORCE_UNSAFE_CONFGIURE=1. As far as I can tell it's not doing anything (it's also misspelled). I didn't notice any different output when I built with the command enabled vs when I deleted it.

Also, just curious why aren't we building the GUI as well? Is it not particularly useful compared to the cli?

pspec_x86_64.xml
87

*.a files are static libaries, which are essentially not allowed in the repos as we only want shared libraries for ease of maintenance.

Basically if we update a package with a static library then every single package that uses that library needs to be recompiled to pick up the updates in that library. Whereas if we use a shared library we can just update the library and every program using that library automatically uses the updated version (because they just link against the updated version).

This is usually easy to fix, just add --disable-static as an argument to the %configure command.

eradot4027 added inline comments.Fri, Jun 11, 11:27 PM
package.yml
17

GUI requires TCLLIB, which doesn't seem to be in the repo or third party. I found a version in the community repo which is discontinued. So, to build the gui I would need package tcllib. But, from what I have found so far seems like it isn't acceptable in the Solus repo. Any suggestions for a work around?

ReillyBrogan added inline comments.Sat, Jun 12, 4:39 PM
package.yml
17

The repos not having TCCLIB is reason enough not to include the GUI IMO.

I was just curious as to the reason.

eradot4027 marked 4 inline comments as done.Tue, Jun 15, 11:32 PM