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

valkey: fix on 10.6, remove brewism #23817

Merged
merged 1 commit into from
May 4, 2024
Merged

Conversation

barracuda156
Copy link
Contributor

Closes: https://trac.macports.org/ticket/69887

Description

Fix build on 10.6, drop unneeded brewism

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.6
Xcode 3.2

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL in commit message?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@macportsbot
Copy link

Notifying maintainers:
@mohd-akram for port valkey.

@macportsbot macportsbot added type: bugfix maintainer: open Affects an openmaintainer port labels May 4, 2024
@mohd-akram
Copy link
Member

Is the check faulty here then? Maybe it can be fixed at the source.

@barracuda156
Copy link
Contributor Author

Is the check faulty here then? Maybe it can be fixed at the source.

It is broken, I think. MAC_OS_X_VERSION_MAX_ALLOWED will probably work, not __MAC_OS_X_VERSION_MAX_ALLOWED.

@barracuda156
Copy link
Contributor Author

Precisely. Their check is doing the opposite of what they thought it is. This works correctly:

#if defined(__APPLE__) && defined(MAC_OS_X_VERSION_MAX_ALLOWED) && MAC_OS_X_VERSION_MAX_ALLOWED >= 1060
#define MAC_OS_10_6_DETECTED
#endif

However, dropping that flag I found another bug in the code, apparently:

debug.c: In function 'getAndSetMcontextEip':
debug.c:1210:28: warning: implicit declaration of function 'arm_thread_state64_get_pc' [-Wimplicit-function-declaration]
 1210 |     void *old_val = (void*)arm_thread_state64_get_pc(uc->uc_mcontext->__ss);
      |                            ^~~~~~~~~~~~~~~~~~~~~~~~~
debug.c:1212:9: warning: implicit declaration of function 'arm_thread_state64_set_pc_fptr' [-Wimplicit-function-declaration]
 1212 |         arm_thread_state64_set_pc_fptr(uc->uc_mcontext->__ss, eip);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from debug.c:31:
debug.c: In function 'logRegisters':
debug.c:1365:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1365 |         (unsigned long) uc->uc_mcontext->__ss.__x[0],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1366:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1366 |         (unsigned long) uc->uc_mcontext->__ss.__x[1],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1367:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1367 |         (unsigned long) uc->uc_mcontext->__ss.__x[2],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1368:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1368 |         (unsigned long) uc->uc_mcontext->__ss.__x[3],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1369:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1369 |         (unsigned long) uc->uc_mcontext->__ss.__x[4],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1370:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1370 |         (unsigned long) uc->uc_mcontext->__ss.__x[5],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1371:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1371 |         (unsigned long) uc->uc_mcontext->__ss.__x[6],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1372:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1372 |         (unsigned long) uc->uc_mcontext->__ss.__x[7],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1373:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1373 |         (unsigned long) uc->uc_mcontext->__ss.__x[8],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1374:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1374 |         (unsigned long) uc->uc_mcontext->__ss.__x[9],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1375:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1375 |         (unsigned long) uc->uc_mcontext->__ss.__x[10],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1376:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1376 |         (unsigned long) uc->uc_mcontext->__ss.__x[11],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1377:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1377 |         (unsigned long) uc->uc_mcontext->__ss.__x[12],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1378:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1378 |         (unsigned long) uc->uc_mcontext->__ss.__x[13],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1379:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1379 |         (unsigned long) uc->uc_mcontext->__ss.__x[14],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1380:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1380 |         (unsigned long) uc->uc_mcontext->__ss.__x[15],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1381:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1381 |         (unsigned long) uc->uc_mcontext->__ss.__x[16],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1382:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1382 |         (unsigned long) uc->uc_mcontext->__ss.__x[17],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1383:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1383 |         (unsigned long) uc->uc_mcontext->__ss.__x[18],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1384:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1384 |         (unsigned long) uc->uc_mcontext->__ss.__x[19],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1385:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1385 |         (unsigned long) uc->uc_mcontext->__ss.__x[20],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1386:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1386 |         (unsigned long) uc->uc_mcontext->__ss.__x[21],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1387:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1387 |         (unsigned long) uc->uc_mcontext->__ss.__x[22],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1388:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1388 |         (unsigned long) uc->uc_mcontext->__ss.__x[23],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1389:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1389 |         (unsigned long) uc->uc_mcontext->__ss.__x[24],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1390:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1390 |         (unsigned long) uc->uc_mcontext->__ss.__x[25],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1391:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1391 |         (unsigned long) uc->uc_mcontext->__ss.__x[26],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1392:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1392 |         (unsigned long) uc->uc_mcontext->__ss.__x[27],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1393:46: error: 'struct __darwin_ppc_thread_state' has no member named '__x'
 1393 |         (unsigned long) uc->uc_mcontext->__ss.__x[28],
      |                                              ^
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1394:25: warning: implicit declaration of function 'arm_thread_state64_get_fp' [-Wimplicit-function-declaration]
 1394 |         (unsigned long) arm_thread_state64_get_fp(uc->uc_mcontext->__ss),
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1395:25: warning: implicit declaration of function 'arm_thread_state64_get_lr' [-Wimplicit-function-declaration]
 1395 |         (unsigned long) arm_thread_state64_get_lr(uc->uc_mcontext->__ss),
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1396:25: warning: implicit declaration of function 'arm_thread_state64_get_sp' [-Wimplicit-function-declaration]
 1396 |         (unsigned long) arm_thread_state64_get_sp(uc->uc_mcontext->__ss),
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
debug.c:1398:47: error: 'struct __darwin_ppc_thread_state' has no member named '__cpsr'; did you mean '__cr'?
 1398 |         (unsigned long) uc->uc_mcontext->__ss.__cpsr
      |                                               ^~~~~~
server.h:3731:27: note: in definition of macro 'serverLog'
 3731 |         _serverLog(level, __VA_ARGS__);\
      |                           ^~~~~~~~~~~
make[1]: *** [debug.o] Error 1
make[1]: *** Waiting for unfinished jobs....

@barracuda156
Copy link
Contributor Author

barracuda156 commented May 4, 2024

Yeah, wait with merging this, it is more broken than I thought. This has to be fixed.

UPD. Error is not because of the flag being dropped, but because of the wrong assumptions for archs. Now once detection of 10.6 is fixed, hell broke loose LOL

@barracuda156 barracuda156 marked this pull request as draft May 4, 2024 10:27
@barracuda156 barracuda156 marked this pull request as ready for review May 4, 2024 12:18
@barracuda156
Copy link
Contributor Author

@mohd-akram Ok, this builds fine for me, and we do not need to drop the flag.
Thanks for pointing out at the problematic check.

@mohd-akram
Copy link
Member

Is the check faulty here then? Maybe it can be fixed at the source.

It is broken, I think. MAC_OS_X_VERSION_MAX_ALLOWED will probably work, not __MAC_OS_X_VERSION_MAX_ALLOWED.

I'm surprised that makes a difference. I'm just curious, could you check what values you get for them in a simple program?

#include <AvailabilityMacros.h>
#include <stdio.h>

int main(void)
{
	printf("MAC_OS_X_VERSION_MIN_REQUIRED=%d\n", MAC_OS_X_VERSION_MIN_REQUIRED);
	printf("__MAC_OS_X_VERSION_MIN_REQUIRED=%d\n", __MAC_OS_X_VERSION_MIN_REQUIRED);
	printf("MAC_OS_X_VERSION_MAX_ALLOWED=%d\n", MAC_OS_X_VERSION_MAX_ALLOWED);
	printf("__MAC_OS_X_VERSION_MAX_ALLOWED=%d\n", __MAC_OS_X_VERSION_MAX_ALLOWED);
	return 0;
}

@barracuda156
Copy link
Contributor Author

It will not compile, since those are undefined :)

36-142% gcc -arch ppc ./test.c -o test
./test.c: In function ‘main’:
./test.c:7: error: ‘__MAC_OS_X_VERSION_MIN_REQUIRED’ undeclared (first use in this function)
./test.c:7: error: (Each undeclared identifier is reported only once
./test.c:7: error: for each function it appears in.)
./test.c:9: error: ‘__MAC_OS_X_VERSION_MAX_ALLOWED’ undeclared (first use in this function)

It is actually a known issue. Confusion arose from a fact that Availability.h. uses underscorred versions.

@mohd-akram
Copy link
Member

I see, and Availability.h, at least on macOS 14, is implicitly included when you include stuff like stdio.h. On newer macOS, it's also included when you include AvailabilityMacros.h, so that's why that code "works" on newer versions, so kind of useless. I don't think the underscored versions are meant to be used by user programs. I think you use them in some of your patches; should probably just stick to the non-underscored always.

@mohd-akram mohd-akram merged commit b5ea3be into macports:master May 4, 2024
4 checks passed
@barracuda156 barracuda156 deleted the valkey branch May 4, 2024 14:24
@barracuda156
Copy link
Contributor Author

@mohd-akram Yes, I learnt this on my own mistakes LOL
And I did not notice the issue initially since it was coupled with a macro excluding ppc (which worked correctly).

Wherever it is still used in our patches, should be changed to non-underscored ones (upstreams might occasionally use Availability.h directly, I think libass does, that works fine then).

@mohd-akram
Copy link
Member

I don't think upstream should ever use them TBH. They actually come from AvailabilityInternal.h (included by Availability.h), and as per the name, they're supposed to be internal. Even Availability.h says in the first line "these macros are for use in OS header files" and Apple code/docs don't use underscored versions.

@kencu
Copy link
Contributor

kencu commented May 4, 2024

For some background, the underscore versions are for Apple internal use. The non-underscore versions are more compatible and are the ones we usually use.

The Tiger SDK does not have Availability.h but does have AvailabilityMacros.h, so we came to use that most of the time. It works fine on newer systems too.

https://github.com/kencu/macos-sdk/tree/master/MacOSX10.4u.sdk/usr/include

I added Availability.h to LegacySupport some years back so it would be there for Tiger if needed.

macports/macports-legacy-support@221ec6c

@mohd-akram
Copy link
Member

I believe MAC_OS_X_VERSION_MIN_REQUIRED is also preferred to MAC_OS_X_VERSION_MAX_ALLOWED. My understanding is that the expectation for the latter is that one would further perform a runtime availability check since the symbol required won't be available when the same binary is run on older versions. In that case, you'd have to fallback to compatible code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer: open Affects an openmaintainer port type: bugfix
4 participants