Details
- Reviewers
- None
- Group Reviewers
Contributor Reviewers (ports) - Commits
- rP458712: New port: x11/polybar
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| x11/polybar/Makefile | ||
|---|---|---|
| 54 ↗ | (On Diff #37029) | I think it would be a good idea to turn all options ON by default, intended for people using pre-built packages. I guess ALSA is the only option what actually bring a dependency, but most people using packages in a desktop setup should already has ALSA installed anyway. |
The '+=' were a lack of understanding on my part, those have been fixed.
I also was not sure if making all dependencies enabled by default was ok,
as it requires a lot of dependencies. Either way, these have been fixed.
WWW has been added and a few spacing issues fixed. Not sure why the
README was in there, thats been removed.
The idea is that the default options will fit for 95%+ of the people, this means that you should enable most things that are useful. The 5%- of people that are left are:
- People who do not want some option (because they're allergic to it, or are short on disk space, or...), and they can disable it.
- People who have some edge case where they need some option that is not on by default because almost nobody needs it, and they can enable it.
Some questions, but nothing serious. You can mark comments as "done" when they're done (e.g. the question about the WWW line) so as to indicate that things are progressing.
| x11/polybar/Makefile | ||
|---|---|---|
| 50 ↗ | (On Diff #37075) | It's a bit strange to me that i3 support doesn't actually use anything from i3 (while IPC does). The wiki page for polybar says jsoncpp + i3 are needed for this option -- and doesn't talk about dependencies for the IPC module at all. |
| x11/polybar/files/patch-cmake_02-opts.cmake | ||
| 9 ↗ | (On Diff #37075) | From a CMake perspective this is a bit round-a-bout: do you need inotify on FreeBSD? Then don't make it an option. For the port, where no option is provided to switch off inotify, it's moot, but still strange. |
The points you make are very true. I have fixed them.
Removed the inotify option in favor of always using it on FreeBSD.
Rearrange I3 dependencies.
I think it may be a good idea to include a pkg-message explaining why the network module does not work (and eventually other I am not aware, or if some depends of a specific/different setup to work). Or, at very least what is know to not work.
Why doesn't it work? And why not fix it instead of adding a pkg-message?
| x11/polybar/Makefile | ||
|---|---|---|
| 12 ↗ | (On Diff #37359) | Already added by USES=localbase |
| 13 ↗ | (On Diff #37359) | Remove. AFAIK there is no such knob and cmake always builds verbosely. |
| 26 ↗ | (On Diff #37359) | Why noninja? If it's necessary, then please add a small comment to the Makefile to explain why. |
| 30 ↗ | (On Diff #37359) | Please remove the last line and just set GH_ACCOUNT=jaagr |
| 40 ↗ | (On Diff #37359) | Just "Volume Control" would be better IMHO. Enable/disable is already implied for options. Same goes for all the other option descriptions. It would be nicer to group the descriptions together too before all the other options helpers. https://www.freebsd.org/doc/en/books/porters-handbook/porting-order-options.html |
| x11/polybar/Makefile | ||
|---|---|---|
| 26 ↗ | (On Diff #37359) | If noninja is not present, the build will not succeed. It gives errors about namespacing issues. Maybe someone could shed light on what this means? I am not familiar with ninja, or what 'noninja' is actually disabling... |
| x11/polybar/Makefile | ||
|---|---|---|
| 26 ↗ | (On Diff #37359) | A while back we switched to using Ninja instead of make for cmake based projects. Build times are a lot better with Ninja usually. libxpp generates some headers and it happens too late when using Ninja. This upstream commit fixes it: https://github.com/jaagr/xpp/commit/64bd57621102d281c4df9f29ee92fd9c083ebebe So we can use jaagr:xpp:1.4.0-1-g64bd576:xpp/lib/xpp in GH_TUPLE to fix it. Regardless of Ninja or not, the build currently fails in Poudriere: -- Searching for xcbgen with python2 -- Searching for xcbgen with python3 -- Searching for xcbgen with python CMake Error at lib/xpp/CMakeLists.txt:55 (message): Missing required python module: xcbgen It looks like lib/xpp/CMakeLists.txt needs some patching to look for python2.7 specifically. |
| x11/polybar/Makefile | ||
|---|---|---|
| 26 ↗ | (On Diff #37359) | s/python2.7/${PYTHON_CMD}/ |
I have fixed the Makefile suggestions, and successfully built the port using pourdiere. I do have two questions.
First, when I ran portlint -A, it complained about the man pages not being compressed. Is there something I need to do to fix that?
Second, I created a variable XPP_VERSION since a particular commit is being checked out. Is there a better way to handle this type of situation?
Since upstream is willing to receive the changes I think it would be better to patch xpp.
I've made a PR a few days ago in your Github. ^^