Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake Subproject Changes #204

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rmorales87atx
Copy link

Hi,

I made some rudimentary changes to the CMake files so that I could include PDCursesMod as a "subproject".

I'm by no means an expert on CMake so I'm hoping I didn't mess anything else up. I'm sure this can be improved as well.

Example that will essentially only build the VT port as a static library:

	option(PDC_WIDE "" ON)
	option(PDC_BUILD_SHARED "" OFF)
	option(PDC_CHTYPE_32 "" OFF)
	option(PDC_OS2_BUILD "" OFF)
	option(PDC_DOS_BUILD "" OFF)
	option(PDC_DOSVGA_BUILD "" OFF)
	option(PDC_DOSVT_BUILD "" OFF)
	option(PDC_SDL2_BUILD "" OFF)
	option(PDC_SDL2_DEPS_BUILD "" OFF)
	option(PDC_NCURSES_BUILD "" OFF)
	option(PDC_DEMOS_BUILD "" OFF)
	add_subdirectory(PDCursesMod)
	list(APPEND LIBS vt_pdcursesstatic)

	# .. more stuff adding onto ${LIBS} ..

	target_link_libraries(my_app ${LIBS})

@Bill-Gray
Copy link
Owner

Thank you; this looks as if it ought to be quite a help for CMake/PDCursesMod users (of whom there appear to be a fair number).

However, I'm not one of them (still using 'traditional' makefiles) and I am in no way able to comment on CMake matters. @jmalak @jwinarske @sherpya @mannyamorim @GitMensch ... this is really your chunk of the code; any thoughts?

@GitMensch
Copy link
Collaborator

It "looks" good to me too, but I normally don't use cmake so I can't say much to this PR (just fixed one conflict).
@rmorales87atx Do we need PDC_DOSVT_BUILD and PDC_VT_BUILD?

Should we have at least one of those "as subprojects" build added to Appveyor for testing (not sure how again...)?

@jmalak @jwinarske As Bill I'd really appreciate an actual review.

@jwinarske
Copy link
Contributor

@GitMensch I can take a look at this in a few weeks. I'm pretty busy right now. This PR requires creating some test cases that don't exist. It's not suitable for merging just yet.

@dmitmel
Copy link

dmitmel commented Feb 21, 2023

@jwinarske Could you please elaborate what test cases you are talking about? I am also interested in seeing this PR merged and am willing to help.

@jwinarske
Copy link
Contributor

@dmitmel I just worked out a better solution. I may be able to submit PR tomorrow.

@dmitmel
Copy link

dmitmel commented Mar 2, 2023

By the way, this PR also needs to change the way compilation settings (include_directories, target_link_libraries etc) are set in cmake/project_common.cmake because currently they are not propagated (set with the PUBLIC modifier, in other words) to dependent targets. In particular I've had problems with include paths and linkage, temporarily fixed with:

target_include_directories(my_target PUBLIC ${pdcurses_SOURCE_DIR})
target_link_libraries(my_target PRIVATE winmm.lib)  # This is just for Windows, obviously

Comment on lines +8 to +9
set(PDCURSES_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
set(PDCURSES_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
Copy link

Choose a reason for hiding this comment

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

Also, these two variables aren't necessary since CMake already automatically creates <PROJECT>_SOURCE_DIR and <PROJECT>_BINARY_DIR, in our case pdcurses_SOURCE_DIR and pdcurses_BINARY_DIR (notice the casing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants