cmake support for NetHack 3.7 #662

heiner · Jan 26, 2022

Hey!

I don't think you will want to immediately (or perhaps ever) merge this, I just thought it might be good to document somewhere how to build NetHack 3.7 via cmake out-of-source. I also have a cmake version for NetHack 3.6 (harder, as it requires dealing with dgn_comp, lvl_comp etc).

This doesn't yet include building the Guildebook or flags like WANT_LIBNH but they would be easy to add if there is interest.

To build:

mkdir build && cd build 
cmake .. -GNinja && ninja && cmake --install .
./nethack

libm math functions are used by lua and NetHack and Lua both need to link to libdl.a.

dextercd · Jan 26, 2022

For me CMake is much better than the Makefiles approach so I'm happy to see this PR. I ran into some minor issues.

I get the following error when using the CMake build:

ninja: error: 'lua/src/fetch-lua/src/liblua.a', needed by 'nethack', missing and no known rule to make it

Applying the following patch seems to help:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 7c2678eb7..de2a4819f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -54,6 +54,7 @@ ExternalProject_Add(
   BUILD_IN_SOURCE TRUE
   CONFIGURE_COMMAND ""
   BUILD_COMMAND make
+  BUILD_BYPRODUCTS <BINARY_DIR>/src/liblua.a
   INSTALL_COMMAND ""
   STEP_TARGETS build)

After that I get this error:

$ cmake --build . -v
[1/136] : && /usr/bin/cc   util/CMakeFiles/makedefs.dir/makedefs.c.o util/CMakeFiles/makedefs.dir/__/src/monst.c.o util/CMakeFiles/makedefs.dir/__/src/objects.c.o util/CMakeFiles/makedefs.dir/__/src/alloc.c.o util/CMakeFiles/makedefs.dir/__/src/date.c.o -o util/makedefs   && :
FAILED: util/makedefs 
: && /usr/bin/cc   util/CMakeFiles/makedefs.dir/makedefs.c.o util/CMakeFiles/makedefs.dir/__/src/monst.c.o util/CMakeFiles/makedefs.dir/__/src/objects.c.o util/CMakeFiles/makedefs.dir/__/src/alloc.c.o util/CMakeFiles/makedefs.dir/__/src/date.c.o -o util/makedefs   && :
/usr/bin/ld: util/CMakeFiles/makedefs.dir/__/src/alloc.c.o: in function `alloc':
alloc.c:(.text+0x34): undefined reference to `panic'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

Applying the following patch seems to help:

diff --git a/util/CMakeLists.txt b/util/CMakeLists.txt
index c7ede9b38..70f1174de 100644
--- a/util/CMakeLists.txt
+++ b/util/CMakeLists.txt
@@ -1,5 +1,5 @@
 set(MAKEDEFS_SRC makedefs.c ${NH_SRC}/monst.c ${NH_SRC}/objects.c
-                 ${NH_SRC}/alloc.c ${NH_SRC}/date.c)
+                 ${NH_SRC}/alloc.c ${NH_SRC}/date.c panic.c)
 
 # TODO: Re-add onames.h once "prob error for class 4 (28%)" error is fixed.
 set(MAKEDEFS_HEADERS ${NH_INC_GEN}/date.h # ${NH_INC_GEN}/onames.h

After this the nethack target gets a bunch of linker errors because of liblua.a uses math functions but is not linking to libm.a. liblua.a and loadlib.c also require it to link to libdl.a for dlopen, dlclose etc.

Applying the following patch fixes the issue.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index de2a4819f..c26c3ce53 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -93,7 +93,7 @@ file(
 
 # nethack executable.
 add_executable(nethack ${NETHACK_SRC})
-target_link_libraries(nethack ncurses ${TOPLUALIB})
+target_link_libraries(nethack ncurses ${TOPLUALIB} ${CMAKE_DL_LIBS} m)
 
 # TODO: Add check-dlb, Guidebook
 add_dependencies(nethack util dat lua_support)

After this nethack compiled cleanly 😄 I pushed these changes here as well: https://github.com/dextercd/NetHack/tree/NetHack-3.7-cmake

CMakeLists.txt

dextercd · Jan 26, 2022

dat/CMakeLists.txt

+add_custom_command(
+  DEPENDS $<TARGET_FILE:makedefs> bogusmon.txt engrave.txt epitaph.txt
+  OUTPUT bogusmon epitaph engrave
+  COMMAND $<TARGET_FILE:makedefs> ARGS -s)


You can use the target name directly in the COMMAND argument ~~which will also automatically register it as a dependency~~ (it seems that that's not actually the case). That's a bit tidier IMO

Can you clarify this comment? Are you suggesting a change like this?

- COMMAND $<TARGET_FILE:makedefs> ARGS -s) + COMMAND makedefs ARGS -s)

I wasn't sure that this would include the full path. But perhaps I am misunderstanding your suggestion.

Yes, that's what I meant. This is a documented feature:

If COMMAND specifies an executable target name (created by the add_executable() command), it will automatically be replaced by the location of the executable created at build time if .. [cross compile details] ..

This is how I use add_custom_command most of the time. Alternatively I put a variable that contains the path to a program that was found via find_program.

The target name syntax could be confused with referring to a program that is on the $PATH. But that's not a common use case in my experience.

I should clarify that this is just a minor nitpick. I've never used TARGET_FILE for this and don't remember seeing it anywhere else either, but it's readable and I'm sure it does the job. 😉

Interesting! But the cmake documentation continues like this:

Arguments to COMMAND may use generator expressions. Use the TARGET_FILE generator expression to refer to the location of a target later in the command line (i.e. as a command argument rather than as the command to execute).

(OK, so in our case it is the command itself, so I guess we shouldn't use TARGET_FILE?)

Whenever one of the following target based generator expressions are used as a command to execute or is mentioned in a command argument, a target-level dependency will be added automatically so that the mentioned target will be built before any target using this custom command (see policy CMP0112).

TARGET_FILE TARGET_LINKER_FILE TARGET_SONAME_FILE TARGET_PDB_FILE

So this style introduces a dependency (which we want in this case).

Overall, cmake is such a highly non-uniform language, it's kind of sad it became standard (over, say, something closer to bazel).

Interesting, I hadn't seen that but some testing reveals that it doesn't quite work how I'd expect.

I think I get it now.. For the COMMAND argument, using either the target name or $<TARGET_FILE:target_name> creates a target-level dependency but neither introduce a file-level dependency.

This target-level dependency does NOT add a file-level dependency that would cause the custom command to re-run whenever the executable is recompiled. List target names with the DEPENDS option to add such file-level dependencies.

File-level dependence is what I had in mind in my previous comments. TBH I didn't know CMake had different kinds of dependencies.

I like CMake but it definitely has its sharp edges (and mushy ones.).

I am not sure I understand the difference between target-level dependencies and file-level dependencies, but I gather the latter means: If a target has a file-level dependency on a binary (file), it will cause that binary file to be build, but if it's already present it is not rebuild?

By the way if memory serves some of the $<TARGET_FILE:target_name> syntax I introduced after I noticed my cmake setup breaks when applied to a NetHack which also has build artefacts in-source (e.g., run make, then run cmake). CMake chose the wrong files to depend on in some cases, this commit tried to work around that, but might have overdone it.

Yes from what I can tell that is the difference between target and file-level dependencies. Rebuilding the data files when the binary has change makes most sense to me.

Hm, that's odd. I may play around with the scenario you described. Was the CMake build an in-source or out-of-source build? Anyhow, keeping it as TARGET_FILE is good. Please consider this conversation resolved

Ah I'm sorry, I wasn't quite clear.

Issues can occur when calling cmake after using NetHacks current/standard Makefile setup, e.g., on Linux:

cd sys/unix sh setup.sh hints/linux cd ../.. make mkdir build && cd build cmake .. -GNinja ninja

The issue is that this causes in-source build artefacts to be picked up by cmake, as opposed to their out-of-source targets. The commit I linked above is trying to work around that; I don't remember if using $<TARGET_FILE:target_name> truly helped with this; what definitely helped was using absolute paths everywhere.

Updates to NetHack 3.7 CMakeLists.txt

heiner · Jan 27, 2022

Thanks @dextercd for your fixes! I'm not sure how this could work for me without this. I've merged your commits into my branch.

dextercd · Jan 27, 2022

Looks like the NetHack-3.7 branch was deleted and recreated, which closes all pull requests targeting this branch.. That was probably not intentional 😕

nhcopier · Jan 27, 2022

Apologies. We had an issue we needed to clean up.

…

On Jan 27, 2022, at 11:27 AM, Dexter Castor Döpping ***@***.***> wrote: Looks like the NetHack-3.7 branch was deleted and recreated, which closes all pull requests targeting this branch.. That was probably not intentional 😕 — Reply to this email directly, view it on GitHub <#662 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD5NCFWHC7SXYIOT5JMY7A3UYFW53ANCNFSM5M4FQW6A>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you modified the open/close state.

This was disliked by a reviewer in NetHack/NetHack#662 but corresponds to what NetHack's Makefiles do.

CMakeLists.txt

dextercd · Feb 1, 2022

dat/CMakeLists.txt

+add_custom_command(
+  DEPENDS $<TARGET_FILE:makedefs> bogusmon.txt engrave.txt epitaph.txt
+  OUTPUT bogusmon epitaph engrave
+  COMMAND $<TARGET_FILE:makedefs> ARGS -s)


Yes from what I can tell that is the difference between target and file-level dependencies. Rebuilding the data files when the binary has change makes most sense to me.

Hm, that's odd. I may play around with the scenario you described. Was the CMake build an in-source or out-of-source build? Anyhow, keeping it as TARGET_FILE is good. Please consider this conversation resolved

Heinrich Kuttler and others added 5 commits January 20, 2022 23:53

CMakeLists.txt files from NetHack 3.6.6.

144214f

CMakeLists.txt for NetHack 3.7.

04a1e47

Specify lua build output

f60f9eb

Add missing panic.c dependency to makedefs

a8eb1f2

Add libdl and libm dependencies

98b7d51

libm math functions are used by lua and NetHack and Lua both need to link to libdl.a.

dextercd reviewed Jan 26, 2022

View reviewed changes

Merge pull request #1 from dextercd/NetHack-3.7-cmake

313bce0

Updates to NetHack 3.7 CMakeLists.txt

List sources without GLOB.

526be62

nhcopier deleted the branch NetHack:NetHack-3.7 January 27, 2022 16:20

nhcopier closed this Jan 27, 2022

nhmall reopened this Jan 27, 2022

heiner pushed a commit to facebookresearch/nle that referenced this pull request Jan 31, 2022

Resolve a NN TODO: glob *.lev files.

c283ca5

This was disliked by a reviewer in NetHack/NetHack#662 but corresponds to what NetHack's Makefiles do.

dextercd approved these changes Feb 1, 2022

View reviewed changes

heiner mentioned this pull request Feb 1, 2022

Updates to cmake facebookresearch/nle#310

Merged

Flone-dnb added a commit to Flone-dnb/NetHack that referenced this pull request Jan 20, 2024

Merge NetHack#662

535f260

Search code, repositories, users, issues, pull requests...

cmake support for NetHack 3.7 #662

Are you sure you want to change the base?

cmake support for NetHack 3.7 #662

Uh oh!

Conversation

heiner commented Jan 26, 2022

Uh oh!

dextercd commented Jan 26, 2022

Uh oh!

Uh oh!

dextercd Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heiner commented Jan 27, 2022

Uh oh!

dextercd commented Jan 27, 2022

Uh oh!

nhcopier commented Jan 27, 2022 via email

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dextercd Jan 26, 2022 •

edited

Loading