chu11 / freeipmi-mirror

Mirror of GNU FreeIPMI Git Repo - http://savannah.gnu.org/projects/freeipmi/. I maintain the upstream of FreeIPMI and can accept Github pull requests.
GNU General Public License v3.0
12 stars 16 forks source link

Fails to build on major of archs with 1.6.13 #70

Closed Fantu closed 7 months ago

Fantu commented 7 months ago

Today I updated Debian package from 1.6.11 to 1.6.13, I tested without error on amd64 but after upload I saw that failed to build on major of archs related to inb and outb: https://buildd.debian.org/status/package.php?p=freeipmi&suite=sid

chu11 commented 7 months ago

hi, does 1.6.12 work? I guess the issue is the recent:

https://github.com/chu11/freeipmi-mirror/commit/379c8569711d808c3366a99d1dfac5e414858800

although it's not clear to me why it would break things.

chu11 commented 7 months ago

oh crud, i guess this went into 1.6.12

https://github.com/chu11/freeipmi-mirror/commit/1f4c948f48b1b3caab1471b15231b0fc9761ffff

could you try and see which one was the problem?

chu11 commented 7 months ago

soo i'm not sure what the problem is. Going back through several older versions there's been three inb/outb configure changes. It's working for me now but did not work a few versions ago on a few systems for me. I guess there's been a lot of gcc changes recently.

Looking at your builder

/usr/bin/ld: ../libfreeipmi/.libs/libfreeipmi.so: undefined reference to `inb'
/usr/bin/ld: ../libfreeipmi/.libs/libfreeipmi.so: undefined reference to `outb'

this is failing in the linker. Suggesting that inb and outb are declared somewhere on those systems, but they can't be found during link time?

It does make sense to me that non-x86_64 platforms wouldn't have inb and outb ... but why are they declared somewhere? Is it possible those declarations are there by mistake?

I admit I'm very confused.

PLMK what you figure out. Maybe we need to do a test compile within configure.ac via AC_TRY_LINK or something similar.

Fantu commented 7 months ago

I tried to do some fast tests with Debian debomatic servers of other archs reverting the related patches but unfortunately at the moment I was unable to do so due to full space or other problems.

I don't know enough to solve this. Have you already checked if can be useful from the full build log of one failed build, for example arm64? https://buildd.debian.org/status/fetch.php?pkg=freeipmi&arch=arm64&ver=1.6.13-1&stamp=1706369643&raw=0

Relating this above in the build I found:

...
checking whether inb is declared... no
checking whether outb is declared... no
...
driver/ipmi-kcs-driver.c:166:22: warning: implicit declaration of function ‘inb’ [-Wimplicit-function-declaration]
  166 | # define _INB(port)  inb (port)
      |                      ^~~
driver/ipmi-kcs-driver.c:537:11: note: in expansion of macro ‘_INB’
  537 |   return (_INB (IPMI_KCS_REG_STATUS (ctx->driver_address, ctx->register_spacing)));
      |           ^~~~
driver/ipmi-kcs-driver.c: In function ‘_ipmi_kcs_read_next’:
driver/ipmi-kcs-driver.c:167:29: warning: implicit declaration of function ‘outb’ [-Wimplicit-function-declaration]
  167 | # define _OUTB(data, port)  outb (data, port)
      |                             ^~~~
driver/ipmi-kcs-driver.c:718:3: note: in expansion of macro ‘_OUTB’
  718 |   _OUTB (IPMI_KCS_CTRL_READ,
      |   ^~~~~
chu11 commented 7 months ago

hmmm ...

checking for iopl... no
checking whether inb is declared... no
checking whether outb is declared... no
<...>
checking whether inb is declared... (cached) no
checking whether outb is declared... (cached) no

so it's not finding inb and outb like we expect.

#elif (defined(HAVE_INB) && defined(HAVE_OUTB)) || (defined(HAVE_DECL_INB) && defined(HAVE_DECL_OUTB))
# define _INB(port)  inb (port)
# define _OUTB(data, port)  outb (data, port)

any way to see what is in that build's config.h file? What is being defined?

chu11 commented 7 months ago

agh ... i think i see the problem

https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Generic-Declarations.html

define HAVE_DECL_symbol (in all capitals) to ‘1’ if symbol is declared, otherwise to ‘0’.

but inb/outb check is defined(HAVE_DECL_INB).

if you revert the commit

379c8569711d808c3366a99d1dfac5e414858800

does everything work?

As I look at it closer, I think there were two commits that went in to solve the same problem (b/c of the mistake of forgetting to announce 1.6.12 release, an additional fix for the same problem went into 1.6.13). I think reverting that commit might be the solution.

If you can confirm it fixes your build, I'll do an emergency 1.6.13-X release or ... maybe 1.6.14 ...

y-martin commented 7 months ago

Hello

I'm the author of the 379c856. I introduce the bug, the fix should be:

#elif (defined(HAVE_INB) && defined(HAVE_OUTB)) || (HAVE_DECL_INB == 1 && HAVE_DECL_OUTB == 1)

On armhf, I have got for INB:

#define HAVE_DECL_INB 0
/* #undef HAVE_INB */
chu11 commented 7 months ago

@y-martin thanks for checking in. were you aware of this fix in 1.6.12? (i forgot to announce the release)

cf18a13fde3d9bb25623f9866e29b575266b2c00

it's similar to yours, wondering if that one works for you instead, then we can revert yours.

Fantu commented 7 months ago

If you can confirm it fixes your build, I'll do an emergency 1.6.13-X release or ... maybe 1.6.14 ...

I suggest avoid -X release as revision normally used by distro packaging, for example Debian and derivates (https://www.debian.org/doc/debian-policy/ch-controlfields.html#version).

y-martin commented 7 months ago

@chu11 I didn't find this fix when I try to understand my issue. You can simply revert 379c856 so.

Regards

y-martin commented 7 months ago

@Fantu so the right fix you can backport is https://github.com/chu11/freeipmi-mirror/commit/cf18a13fde3d9bb25623f9866e29b575266b2c00 instead in bookworm build (if you can)

On amd64, config/config.h shows me:

grep HAVE_INB config/config.h
#define HAVE_INB 1

so inb() will be used and tools will work on kcs ipmi hardware.

Fantu commented 7 months ago

@y-martin have you tried if full freeipmi package build with revert of your commit completed with success?

y-martin commented 7 months ago

Hello @Fantu

Yes ! Build of 1.6.10 is ok on amd64:

grep HAVE_INB config/config.h
#define HAVE_INB 1

ipmi-fru/ipmi-fru -V
ipmi-fru - 1.6.10

Sorry for my contrib. Thanks a lot guys !

Fantu commented 7 months ago

@y-martin can you please try to build on arm of 1.6.13-1 package plus revert of your patch?

y-martin commented 7 months ago

@y-martin can you please try to build on arm of 1.6.13-1 package plus revert of your patch?

Build is ok too.

chu11 commented 7 months ago

@fantu i've reverted the patch in the stable branch. want to try real quick to make sure it's happy on all debian systems? If so then I can do a release ASAP (hopefully today ... if not today, very soon)

https://github.com/chu11/freeipmi-mirror/tree/freeipmi-1-6-0-stable

Fantu commented 7 months ago

Uploaded to Debian with https://github.com/chu11/freeipmi-mirror/commit/0e7181250f2a867c7ab81206d2c7f123074d62a7 and build done for now was completed https://buildd.debian.org/status/package.php?p=freeipmi&suite=sid

chu11 commented 7 months ago

cool, thanks everyone release will happen in the next handful of minutes