Page MenuHomeSolus

Initial commit of GNU poke tui
Needs ReviewPublic

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

Details

Summary

Intial commit of GNU poke tui

Closes T9618
Depends on T9785

Test Plan

Tested on Solus Budgie while reviewing and binary file

Diff Detail

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

Event Timeline

eradot4027 created this revision.Jun 11 2021, 1:25 AM
eradot4027 requested review of this revision.Jun 11 2021, 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
15–16

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

18

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
88

*.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.Jun 11 2021, 11:27 PM
package.yml
18

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.Jun 12 2021, 4:39 PM
package.yml
18

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.Jun 15 2021, 11:32 PM
JoshStrobl requested changes to this revision.Aug 23 2021, 1:29 PM
JoshStrobl added a subscriber: JoshStrobl.

Patch summary needs to be appropriately updated and you need to squash your commits and do a proper update of the patch. We have documentation on this via the Submitting The Package doc.

This revision now requires changes to proceed.Aug 23 2021, 1:29 PM

Any update on fixing the issues pointed out @eradot4027 ?

maikwoehl edited the summary of this revision. (Show Details)Sep 29 2021, 10:43 PM

I've felt free to add the Closes and Depends part into the summary.

@JoshStrobl: If I'm right the 'Commits' tab should not show any commits, right?

And I would suggest to put that into programming.tools?

So if @eradot4027 has no time to step up until 2021-10-10, I would ask you @JoshStrobl if I could force update the diff with a cleaned up patch? Just to get that sorted in the next time?

@maikwoehl I would love to put this across the line. As a newbie I have misplaced my original git repo or was it deleted as it was marked abandoned. If I can get access to it, I would love to complete the work and learn so that I can better able to contribute in the future.

maikwoehl added a comment.EditedSep 30 2021, 9:15 PM

@maikwoehl I would love to put this across the line. As a newbie I have misplaced my original git repo or was it deleted as it was marked abandoned. If I can get access to it, I would love to complete the work and learn so that I can better able to contribute in the future.

I'm a newbie, too.

You can just create the repo again, get the first diff and the second diff and patch your local repo. Then you can arc diff --update D11259 to overwrite the existing differential and link your local commit with this differential again: https://secure.phabricator.com/book/phabricator/article/arcanist_diff/.

Closes T9618
Depends on T9785

eradot4027 updated this revision to Diff 29029.Oct 1 2021, 12:27 AM

Added abi files

eradot4027 updated this revision to Diff 29030.Oct 1 2021, 9:07 AM
eradot4027 marked an inline comment as not done.

Resolved symbol and library issue.

eradot4027 updated this revision to Diff 29090.Oct 3 2021, 8:25 PM

fixed symbols in abi*

eradot4027 updated this revision to Diff 29098.Oct 3 2021, 10:40 PM

This should resolve all final issues with package.yml and update the abi* files correctly

eradot4027 updated this revision to Diff 29099.Oct 3 2021, 11:23 PM

All git squashed

eradot4027 marked an inline comment as done.Oct 3 2021, 11:25 PM