[Misc] A hodgepodge of tiny changes #1146

grendello · 2020-11-28T20:09:12Z

CMake:
Simplify cmake target+version config generation and make it actually
work. With the changes it is now possible to detect and use
Notcurses in the following way:

 find_package(Notcurses REQUIRED)
 ...
 target_link_libraries(myapp PRIVATE notcurses::notcurses)

Also, added the same CMake configuration for Notcurses++, to be used
in the following way:

 find_package(Notcurses REQUIRED
 find_package(Notcurses++ REQUIRED)
 ...
 target_link_libraries(myapp PRIVATE notcurses++::notcurses++)

Docs:
notcurses_cell(3): cell_styles_{on,off} -> cell_{on,off}_styles
and cell_load_simple -> cell_load_char

C++ API:

Plane: added constructors taking ncplane_options const& instead of
the multitude of individual parameters
Plane: drop struct when ncplane_options is used.
Plane: added strdup (cell_strdup)
Plane: added extract (cell_extract)

CMake: Simplify cmake target+version config generation and make it actually work. With the changes it is now possible to detect and use `Notcurses` in the following way: find_package(Notcurses REQUIRED) ... target_link_libraries(myapp PRIVATE notcurses::notcurses) Also, added the same CMake configuration for `Notcurses++`, to be used in the following way: find_package(Notcurses REQUIRED find_package(Notcurses++ REQUIRED) ... target_link_libraries(myapp PRIVATE notcurses++::notcurses++) Docs: `notcurses_cell(3)`: `cell_styles_{on,off} -> cell_{on,off}_styles` and `cell_load_simple` -> `cell_load_char` C++ API: * Plane: added constructors taking `ncplane_options const&` instead of the multitude of individual parameters * Plane: drop `struct` when `ncplane_options` is used. * Plane: added `strdup` (`cell_strdup`) * Plane: added `extract` (`cell_extract`)

dankamongmen · 2020-11-28T22:41:04Z

Will the existing CMake "${Notcurses_INCLUDE_DIRS}" continue to work? For instance in the growlight CMakeLists.txt I've got:

find_package(Notcurses 2.0.5 CONFIG)
set_package_properties(Notcurses PROPERTIES TYPE REQUIRED)
...
  "${Notcurses_INCLUDE_DIRS}"
  "${Notcurses_LIBRARY_DIRS}"
  "${Notcurses_LIBRARIES}"

and would be loathe to break anyone's working CMake configurations. I figure it will, but just checking.

I guess I can test this easily enough myself.

dankamongmen · 2020-11-28T22:42:12Z

Everything else looks great, thanks! Take a look at #1143 if you can, please.

dankamongmen · 2020-11-28T22:44:05Z

doc/man/man3/notcurses_cell.3.md

@@ -56,9 +56,9 @@ typedef struct cell {

 **unsigned cell_styles(const cell* ***c***);**

-**void cell_styles_on(cell* ***c***, unsigned ***stylebits***);**
+**void cell_on_styles(cell* ***c***, unsigned ***stylebits***);**


Ugh. I'd rather switch cell_{on, off}_styles in the actual API to match ncplane_styles_*, but Allah, the All-Powerful, has fucked us once again. Guess I just missed this in 2.0 API review. Bah! Thanks a lot for this find.

I'm going to go ahead and fix it in USAGE.md as well.

Got it in USAGE.md. cell_load_char() was already correct there.

dankamongmen · 2020-11-28T22:45:38Z

include/ncpp/Plane.hh

 				.y = yoff,
-			  .x = xoff,


dankamongmen · 2020-11-28T22:45:51Z

include/ncpp/Plane.hh

 				throw init_error ("Notcurses failed to create an aligned plane");
+			}


dankamongmen

i've picked up your man page fixes and replicated them in USAGE.md. everything looks good.

grendello · 2020-11-28T22:57:18Z

Will the existing CMake "${Notcurses_INCLUDE_DIRS}" continue to work? For instance in the growlight CMakeLists.txt I've got:
find_package(Notcurses 2.0.5 CONFIG)
set_package_properties(Notcurses PROPERTIES TYPE REQUIRED)
...
  "${Notcurses_INCLUDE_DIRS}"
  "${Notcurses_LIBRARY_DIRS}"
  "${Notcurses_LIBRARIES}"
and would be loathe to break anyone's working CMake configurations. I figure it will, but just checking.

I guess I can test this easily enough myself.

It should work, but the namespace::component notation is so much less verbose - it sets up everything for you :)

dankamongmen · 2020-11-28T23:06:06Z

It should work, but the namespace::component notation is so much less verbose - it sets up everything for you :)

oh sure, and i'll likely update my own cmake configs, just i don't want "FUCKED UP MY WORKING CMAKE BUILD" on my tombstone

dankamongmen · 2020-11-28T23:08:41Z

dankamongmen · 2020-11-28T23:14:58Z

oh there is an unfortunate ambiguity here. i am not saying that cmake worked following an ass-fucking; on the contrary, this tombstone claims that by breaking the speaker's once-working cmake config, i fucked them in the ass

grendello · 2020-11-28T23:16:34Z

It should work, but the namespace::component notation is so much less verbose - it sets up everything for you :)

oh sure, and i'll likely update my own cmake configs, just i don't want "FUCKED UP MY WORKING CMAKE BUILD" on my tombstone

Hey, shit happens, better things replace worse things :)

dankamongmen · 2020-11-28T23:23:25Z

Hey, shit happens, better things replace worse things :)

while we have not yet conquered the world, and I'm still waiting for my Turing award and parade, we have acquired real users, real as in real reverse-deps in distributions that you've heard of, and when i break existing stuff and trigger debian/fedora FTBFS (failed-to-build-from-source) bugs....well, not sharing a border with them, i take taciturn German DBTS reports very seriously and am made to feel a wretched failure of a fat, loud american.

the upstream authors deal with these changes with panache and elan. but debian gets more unhappy than you can imagine when this happens, and pathetic vain me is aiming to become a Debian Developer (from Maintainer) sometime this life, and black eyes like that do not help my case.

besides that, cmake sucks, and breaking it seems especially unwelcome.

but yeah, my stuff above is all driven off pkg-config, and won't suffer from your new stuff at all. thanks mang! w00t

grendello · 2020-11-29T00:03:33Z

Hey, shit happens, better things replace worse things :)

while we have not yet conquered the world, and I'm still waiting for my Turing award and parade, we have acquired real users, real as in real reverse-deps in distributions that you've heard of, and when i break existing stuff and trigger debian/fedora FTBFS (failed-to-build-from-source) bugs....well, not sharing a border with them, i take taciturn German DBTS reports very seriously and am made to feel a wretched failure of a fat, loud american.

Well, it seems my (untested) assumption was wrong - the new way does not add the variables you mentioned earlier. Doh. I'll open a new PR to partially revert the changes, so that Notcurses uses the old ways, but Notcurses++ uses the new one (since it had none before, there's no backward compat issue). Better is the enemy of good, eh? :)
I wish I noticed this stuff earlier :(

the upstream authors deal with these changes with panache and elan. but debian gets more unhappy than you can imagine when this happens, and pathetic vain me is aiming to become a Debian Developer (from Maintainer) sometime this life, and black eyes like that do not help my case.

I was a DD for 10 years... It was fun :)

besides that, cmake sucks, and breaking it seems especially unwelcome.

Nah, it's ok. As any build/metabuild system it has its down and upsides. I rather like it over autotools or meson

but yeah, my stuff above is all driven off pkg-config, and won't suffer from your new stuff at all. thanks mang! w00t

I will still open the partial revert PR. You're right that we don't know how many folks out there use pkg-config and how many use cmake and find_package

grendello · 2020-11-29T00:07:32Z

Bummer, I need to revert the Notcurses++ change as well, as it won't work without the Notcurses part... I'll convert the code to your old style. One day we should probably implement FindNotcurses and FindNotcurses++ modules, much more flexible.

Partially reverts: c5c9432 Context: dankamongmen#1146 The above commit implemented the much more convenient way of finding and using a package from CMake, however it broke compatibility with the way id had been before, which defined various `Notcurses_*` variables. Revert the `Notcurses` component to the old way and add support for `Notcurses++` in the same fashion. Perhaps one day we should think of implementing `find_package` support by way of a module, which should make it possible to have the cake and eat the cake. One day.

Partially reverts: c5c9432 Context: #1146 The above commit implemented the much more convenient way of finding and using a package from CMake, however it broke compatibility with the way id had been before, which defined various `Notcurses_*` variables. Revert the `Notcurses` component to the old way and add support for `Notcurses++` in the same fashion. Perhaps one day we should think of implementing `find_package` support by way of a module, which should make it possible to have the cake and eat the cake. One day.

grendello requested a review from dankamongmen November 28, 2020 20:09

dankamongmen reviewed Nov 28, 2020

View reviewed changes

include/ncpp/Plane.hh

.y = yoff,

.x = xoff,

Copy link

Owner

dankamongmen Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad

dankamongmen reviewed Nov 28, 2020

View reviewed changes

include/ncpp/Plane.hh

throw init_error ("Notcurses failed to create an aligned plane");

}

Copy link

Owner

dankamongmen Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

dankamongmen approved these changes Nov 28, 2020

View reviewed changes

dankamongmen merged commit c5c9432 into dankamongmen:master Nov 28, 2020

grendello mentioned this pull request Nov 29, 2020

[CMake] Restore old way of importing Notcurses into a cmake project #1149

Merged

dankamongmen added a commit that referenced this pull request Feb 23, 2021

tree PoC: plutonium high-neutron beta decays #1146

2fd2db7

RMZeroFour mentioned this pull request Nov 7, 2024

cmake: export targets in config files #2800

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Misc] A hodgepodge of tiny changes #1146

[Misc] A hodgepodge of tiny changes #1146

grendello commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

dankamongmen Nov 28, 2020

dankamongmen Nov 28, 2020

dankamongmen Nov 28, 2020

dankamongmen Nov 28, 2020

dankamongmen left a comment

grendello commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

grendello commented Nov 28, 2020

dankamongmen commented Nov 28, 2020 •

edited

Loading

grendello commented Nov 29, 2020

grendello commented Nov 29, 2020

		throw init_error ("Notcurses failed to create an aligned plane");
		}

[Misc] A hodgepodge of tiny changes #1146

[Misc] A hodgepodge of tiny changes #1146

Conversation

grendello commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

dankamongmen Nov 28, 2020

Choose a reason for hiding this comment

dankamongmen Nov 28, 2020

Choose a reason for hiding this comment

dankamongmen Nov 28, 2020

Choose a reason for hiding this comment

dankamongmen Nov 28, 2020

Choose a reason for hiding this comment

dankamongmen left a comment

Choose a reason for hiding this comment

grendello commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

dankamongmen commented Nov 28, 2020

grendello commented Nov 28, 2020

dankamongmen commented Nov 28, 2020 • edited Loading

grendello commented Nov 29, 2020

grendello commented Nov 29, 2020

dankamongmen commented Nov 28, 2020 •

edited

Loading