Ticket #698 (closed patch: fixed)

Opened 6 years ago

Last modified 6 years ago

[PATCH] Mac OS X build fix

Reported by: rusabd Owned by: NotFound
Priority: normal Milestone:
Component: configure Version: 1.2.0
Severity: medium Keywords:
Cc: infinoid Language:
Patch status: applied Platform:

Description

#39033 produce this error:

c++ -o dynlexpad.bundle dynlexpad.o -Wl,-L /Users/rus/dev/parrot/blib/lib -Wl,-search_paths_first -Wl,-syslibroot,/Developer/SDKs/MacOSX10.4u.sdk -mmacosx-version-min=10.3 -L/Users/rus/dev/parrot/blib/lib -L/opt/local/lib -L/sw/lib -L/Users/rus/dev/parrot/blib/lib -undefined dynamic_lookup -bundle -L/Users/rus/dev/parrot/blib/lib -lparrot -lpthread -lm -L/opt/local/lib -licuuc -licudata -lpthread -lm -lm -lgmp -lreadline -lintl ld: in /Users/rus/dev/parrot/blib/lib, can't map file, errno=22 collect2: ld returned 1 exit status make[1]: *** [dynlexpad.bundle] Error 1 make: *** [dynpmc.dummy] Error 2

Attachments

macosbuildfix.patch Download (0.8 KB) - added by rusabd 6 years ago.
tt698-simplify-1.patch Download (1.9 KB) - added by doughera 6 years ago.
tt698-simplify-2.patch Download (8.3 KB) - added by doughera 6 years ago.

Change History

Changed 6 years ago by rusabd

  Changed 6 years ago by NotFound

  • owner set to NotFound
  • status changed from new to assigned
  • component changed from none to configure
  • patch set to applied

Thanks, applied in r39042, please verify the problem is solved.

follow-ups: ↓ 3 ↓ 4   Changed 6 years ago by doughera

Thanks for the patch. Actually, though, if you look at the output above, you'll see that the term that caused you trouble was redundant anyway, since there's another -L/Users/rus/dev/parrot/blib/lib further along in the command line. The #IF(!cygwin and cc==gcc) conditional is strange too. It fails to match if your compiler isn't named exactly 'gcc'. Obvious examples of that include cases where the default 'cc' on a system is actually gcc, cases where you specify a particular version (e.g. gcc-4.2) and cases where you use 'ccache gcc'.

More directly, it's unnecessarily trying to reproduce Configure.pl's logic in setting up the variable libparrot_ldflags. Configure.pl already sets that variable correctly (or if it's incorrect, it should be fixed in Configure.pl). The Makefile also redundantly tries to set up libparrot_ldflags based on #IF(parrot_is_shared). But Configure.pl already should be doing that correctly.

The first patch I have attached, https://trac.parrot.org/parrot/attachment/ticket/698/tt698-simplify-1.patch , simplifies this considerably. The logic is all pushed back to Configure to determine the correct value of libparrot_ldflags. For most of the common platforms, it should already be getting it right.

If you look more carefully, however, I think it's reasonable to question whether it's even appropriate to be building the shared library with -lparrot in the first place. On most systems, I suspect it's not. (In perl 5, for example, dynamic extensions are not normally linked against -lperl.) Hence libparrot_ldflags should normally be empty. (I didn't want to delete it in case there are platforms where it's necessary or appropriate to link against -lparrot. Such platforms should set libparrot_ldflags in the appropriate hints file.)

Unfortunately, libparrot_ldflags is actually used in two very different ways within the parrot source. It is used both for building dynamically-loadable objects and for linking executables. That's a mistake. Those are two separate operations. In config/init/defaults.pm, those two functions are defined by the variables 'ld' and 'link' respectively. Accordingly, there should be two such variables: libparrot_ldflags (used for building dynamically-loadable objects) and libparrot_linkflags (used for linking executables).

The second patch I have attached, https://trac.parrot.org/parrot/attachment/ticket/698/tt698-simplify-2.patch renames the existing libparrot_ldflags to libparrot_linkflags, to match its actual usage as an argument to the 'link' command. I have also invented a new variable, libparrot_ldflags, which is normally empty, which is to be used with the 'ld' command. Where hints files explicitly set the old libparrot_ldflags, I duplicated the setting, because that's what was effectively happening anyway. Platform experts are welcome to correct and fine tune these settings, now that this is possible.

Most of the size of the second patch is that renaming. The remainder is the "simplify" part. Since the new libparrot_ldflags is usually empty, the bug in this ticket is fixed. Also, since the dyn*/*.so files are now no longer linked against -lparrot, TT #540 can be resolved. Also, since TT #540 can be resolved, the extra unnecessary 'disable_rpath' code in TT #476 can be reverted.

There are still some places in the parrot source that confuse 'ld' with 'link' (e.g. examples/embed/Makefile). Also, I haven't looked at mk_language_shell.pl to see what it's doing.

I also recognize that languages outside of the parrot core may have been relying on libparrot_ldflags, and this change could, in principle, break their build process. I think it should be done anyway, because the current process is wrong and if it is maintained, external languages will eventually be forced to re-invent this patch to work around the brokenness.

Changed 6 years ago by doughera

Changed 6 years ago by doughera

in reply to: ↑ 2   Changed 6 years ago by coke

Replying to doughera:

I also recognize that languages outside of the parrot core may have been relying on libparrot_ldflags, and this change could, in principle, break their build process. I think it should be done anyway, because the current process is wrong and if it is maintained, external languages will eventually be forced to re-invent this patch to work around the brokenness.

I don't think there any languages outside of the core building properly against an installed parrot. (partcl is still struggling with that, though we're getting very close now.); Fixing broken behavior here gets a +1 from me.

Additionally I think the -lparrot issue might be what's causing: https://trac.parrot.org/parrot/ticket/627 on OS X.

in reply to: ↑ 2 ; follow-up: ↓ 5   Changed 6 years ago by Infinoid

  • cc infinoid added

Replying to doughera:

The first patch I have attached, https://trac.parrot.org/parrot/attachment/ticket/698/tt698-simplify-1.patch , simplifies this considerably. The logic is all pushed back to Configure to determine the correct value of libparrot_ldflags. For most of the common platforms, it should already be getting it right.

Thanks, applied as r39044. (r39043 reverts the previous change.)

The second patch I have attached, https://trac.parrot.org/parrot/attachment/ticket/698/tt698-simplify-2.patch renames the existing libparrot_ldflags to libparrot_linkflags, to match its actual usage as an argument to the 'link' command. I have also invented a new variable, libparrot_ldflags, which is normally empty, which is to be used with the 'ld' command. Where hints files explicitly set the old libparrot_ldflags, I duplicated the setting, because that's what was effectively happening anyway. Platform experts are welcome to correct and fine tune these settings, now that this is possible.

Thanks, applied as r39045.

Does everything work now for all parties involved?

Mark

in reply to: ↑ 4 ; follow-up: ↓ 6   Changed 6 years ago by rusabd

Replying to Infinoid:

Thanks, applied as r39045. Does everything work now for all parties involved? Mark

Cool, it works perfectly fine

in reply to: ↑ 5   Changed 6 years ago by Infinoid

  • status changed from assigned to closed
  • resolution set to fixed

Replying to rusabd:

Cool, it works perfectly fine

Great, I'll close this ticket. Any further issues/updates can occur in a new ticket. Thanks!

Mark

Note: See TracTickets for help on using tickets.