Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Fix #297 #299

Merged
merged 1 commit into from
Jan 21, 2022
Merged

Fix #297 #299

merged 1 commit into from
Jan 21, 2022

Conversation

heiner
Copy link
Contributor

@heiner heiner commented Jan 20, 2022

NetHack redefined __warn_unused_result__ in tradstdc.h which is really a bit naughty. Fixed upstream in NH 3.7 in NetHack/NetHack@1cb5dc0, cherry-picked here.

Fixes #297.

NetHack was trying to suppress warn_unused_result
in include/tradstdc.h, by defining warn_unused_result
to an empty string. That began causing a build error
in a system-supplied header file cdefs.h
when using 20.10 ubuntu impish.

Try skipping that in tradstdc.h for any linux, unless
the NetHack build defines GCC_URWARN to force it into
play.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 20, 2022
@heiner heiner requested a review from cdmatters January 20, 2022 19:23
@heiner heiner mentioned this pull request Jan 20, 2022
@heiner heiner merged commit 7662821 into main Jan 21, 2022
@heiner heiner deleted the heiner/fix-impish-build-error branch January 21, 2022 11:34
@heiner heiner mentioned this pull request Jan 21, 2022
be11ng pushed a commit to be11ng/nethack-el that referenced this pull request Jun 2, 2022
There was an issue in which, during compilation, this would crop up:
```
In file included from /usr/include/features.h:490,
                 from /usr/include/bits/libc-header-start.h:33,
                 from /usr/include/stdio.h:27,
                 from ../include/global.h:9,
                 from ../include/config.h:588,
                 from monst.c:6:
/usr/include/sys/cdefs.h:405:73: error: macro "__has_attribute" requires an identifier
  405 | #if __GNUC_PREREQ (3,4) || __glibc_has_attribute (__warn_unused_result__)
      |                                                                         ^
```

I should note that this is something that cropped up months ago; this fix was
cherry-picked onto the 3.6 branch back in 2022-01-23.  But I've been at school
so eh.

So this is a long-noticed issue that I'm only just cherry picking in now.
https://nethack.org/v366/bugs.html tracks this under S366-2, and notes that the
issue was with glibc.

This was addressed in the 3.7 branch in Nethack/Nethack#1cb5dc0:
> Work around ubuntu 20.10 build issue
>
> NetHack was trying to suppress warn_unused_result
> in include/tradstdc.h, by defining warn_unused_result
> to an empty string. That began causing a build error
> in a system-supplied header file cdefs.h
> when using 20.10 ubuntu impish.
>
> Try skipping that in tradstdc.h for any linux, unless
> the NetHack build defines GCC_URWARN to force it into
> play.

It was cherry picked in Nethack/Nethack#4a3d466, the documentation was added in
Nethack/Nethack#1b7ad11 and NetHack/NetHack#59b117c.

Also, and this is how I ended up finding out what was going on,
facebookresearch/nle#297 reports just this issue.  It's fixed by
facebookresearch/nle#299 by just straight up cherry picking in that commit.
Maybe I should watch that project more carefully to figure out what I should be
doing.  Maybe I should have just built this on top of that already existing and
honestly probably better API...

I've had to change `mkpatch` to include `include/tradstdc.h`, where the change
is located.  The change itself is quite trivial.

The Lisp version has also been bumped to 0.13.2.  The earliest compatible
version is still 0.12.0.

What bothers me is that when the time comes to do work with 3.7 NetHack, I'll
need to take things out of `mkpatch`.  Just a little icky, and something to
remember.

As an aside, I do vaguely remember quite a few things breaking at the time
though (I use Arch).  So no, not a coincidence, glibc updates are funky.
be11ng pushed a commit to be11ng/nethack-el that referenced this pull request Jun 2, 2022
There was an issue in which, during compilation, this would crop up:
```
In file included from /usr/include/features.h:490,
                 from /usr/include/bits/libc-header-start.h:33,
                 from /usr/include/stdio.h:27,
                 from ../include/global.h:9,
                 from ../include/config.h:588,
                 from monst.c:6:
/usr/include/sys/cdefs.h:405:73: error: macro "__has_attribute" requires an identifier
  405 | #if __GNUC_PREREQ (3,4) || __glibc_has_attribute (__warn_unused_result__)
      |                                                                         ^
```

I should note that this is something that cropped up months ago; this fix was
cherry-picked onto the 3.6 branch back in 2022-01-23.  But I've been at school
so eh.

So this is a long-noticed issue that I'm only just cherry picking in now.
https://nethack.org/v366/bugs.html tracks this under S366-2, and notes that the
issue was with glibc.

This was addressed in the 3.7 branch in NetHack/NetHack@1cb5dc0:
> Work around ubuntu 20.10 build issue
>
> NetHack was trying to suppress warn_unused_result
> in include/tradstdc.h, by defining warn_unused_result
> to an empty string. That began causing a build error
> in a system-supplied header file cdefs.h
> when using 20.10 ubuntu impish.
>
> Try skipping that in tradstdc.h for any linux, unless
> the NetHack build defines GCC_URWARN to force it into
> play.

It was cherry picked in NetHack/NetHack@4a3d466, the documentation was added in
NetHack/NetHack@1b7ad11 and NetHack/NetHack@59b117c.

Also, and this is how I ended up finding out what was going on,
facebookresearch/nle#297 reports just this issue.  It's fixed by
facebookresearch/nle#299 by just straight up cherry picking in that commit.
Maybe I should watch that project more carefully to figure out what I should be
doing.  Maybe I should have just built this on top of that already existing and
honestly probably better API...

I've had to change `mkpatch` to include `include/tradstdc.h`, where the change
is located.  The change itself is quite trivial.

The Lisp version has also been bumped to 0.13.2.  The earliest compatible
version is still 0.12.0.

What bothers me is that when the time comes to do work with 3.7 NetHack, I'll
need to take things out of `mkpatch`.  Just a little icky, and something to
remember.

As an aside, I do vaguely remember quite a few things breaking at the time
though (I use Arch).  So no, not a coincidence, glibc updates are funky.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't install on Linux
4 participants