Details
- Reviewers
JoshStrobl - Group Reviewers
Triage Team - Maniphest Tasks
- T9618: GNU poke - an interactive, extensible editor for binary data.
Tested on Solus Budgie while reviewing and binary file
Diff Detail
- Branch
- main
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 1926 Build 1926: arc lint + arc unit
Event Timeline
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 ↗ | (On Diff #27174) | You can actually drop both of these packages. They're part of system.base and are thus always installed in the build environment. |
| 17 ↗ | (On Diff #27174) | 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. | |
| package.yml | ||
|---|---|---|
| 17 ↗ | (On Diff #27174) | 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? |
| package.yml | ||
|---|---|---|
| 17 ↗ | (On Diff #27174) | The repos not having TCCLIB is reason enough not to include the GUI IMO. I was just curious as to the reason. |
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.
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.
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/.
This should resolve all final issues with package.yml and update the abi* files correctly