Ticket #1130 (closed bug: fixed)

Opened 12 years ago

Last modified 11 years ago

dependency problem for libnci_test

Reported by: doughera Owned by: jkeenan
Priority: normal Milestone:
Component: build Version: trunk
Severity: medium Keywords:
Cc: Language:
Patch status: Platform:

Description

Revision r41942 added $(ALL_PARROT_LIBS) to the ld line for libnci_test. There are three problems with this:

1. The LIBNCI_TEST target doesn't have a dependency on libparrot, so make can try to build LIBNCI_TEST before libparrot is available.

2. The LIBNCI_TEST target is not built with appropriate rpath incantations (i.e. rpath_blib) so the test suite might not be able to find the shared libparrot. This would show up as failures in the t/pmc/nci.t tests. (To fix it to include rpath correctly would require building two versions of libnci_test.so -- one for use in the build directory (with rpath_blib), and one for installation (built with rpath_lib).

3. All those gyrations seem unnecessary. Does libnci_test.so really need to link with -lparrot? It didn't in parrot-1.6.0. The svn log comment mentions that this was a problem on Win32, but I don't know why it wasn't a problem earlier. Not knowing much about Win32 dynamic linking, and not knowing anything about the specific problem, I am hesitant to speculate.

Change History

  Changed 12 years ago by coke

On Wed, Oct 21, 2009 at 3:30 PM, Parrot <parrot-tickets@lists.parrot.org> wrote:
>  3.  All those gyrations seem unnecessary.  Does libnci_test.so really need
>  to link with -lparrot?

Isn't this file JUST for testing? We should only be using that when
testing, not in the core executable.


-- 
Will "Coke" Coleda

  Changed 12 years ago by doughera

On Wed, 21 Oct 2009, Will Coleda wrote:

> On Wed, Oct 21, 2009 at 3:30 PM, Parrot <parrot-tickets@lists.parrot.org> wrote:
> >  3.  All those gyrations seem unnecessary.  Does libnci_test.so really need
> >  to link with -lparrot?
> 
> Isn't this file JUST for testing? We should only be using that when
> testing, not in the core executable.

I wouldn't know.  It *does* get installed, but that might be a different 
mistake.

-- 
    Andy Dougherty		doughera@lafayette.edu

  Changed 12 years ago by doughera

As of r42040, the Makefile now includes the LIBPARROT dependency. This fixes the build order problem, but fails to address the rpath issue. As a specific example, this causes most of the t/pmc/nci.t tests to fail on a clean install of NetBSD-5.0.1. I still don't understand why it was deemed necessary to link with -lparrot in the first place.

follow-up: ↓ 5   Changed 12 years ago by doughera

As of r42376, the Makefile now gets rid of the link with -lparrot, but still includes the LIBPARROT dependency. The LIBPARROT dependency can now go.

in reply to: ↑ 4   Changed 11 years ago by jkeenan

  • status changed from new to assigned
  • owner set to jkeenan

Replying to doughera:

As of r42376, the Makefile now gets rid of the link with -lparrot, but still includes the LIBPARROT dependency. The LIBPARROT dependency can now go.

Coming upon this old ticket tonight, I wrote a patch which I believe implements what doughera was suggesting.

Index: config/gen/makefiles/root.in
===================================================================
--- config/gen/makefiles/root.in        (revision 49073)
+++ config/gen/makefiles/root.in        (working copy)
@@ -2533,7 +2533,7 @@
 src/nci_test$(O): $(PARROT_H_HEADERS) src/nci_test.c
        $(CC) $(CFLAGS) @optimize::src/nci_test.c@ 
        @ccwarn::src/nci_test.c@ @cc_shared@ -I$(@D) 
        @cc_o_out@$@ -c src/nci_test.c
 
-$(LIBNCI_TEST_SO): src/nci_test$(O) $(LIBPARROT)
+$(LIBNCI_TEST_SO): src/nci_test$(O) 
        $(LD) $(LD_LOAD_FLAGS) @ncilib_link_extra@ $(LDFLAGS) \
     @ld_out@$@ src/nci_test$(O) $(C_LIBS)
 

It has passed make test on Linux/i386 (see  this Smolder report) and  also on Darwin/PPC.

Assuming it PASSes, I would like to apply this after this week's release, and then close this ticket. Any objections?

Thank you very much.

kid51

  Changed 11 years ago by jkeenan

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

Patch applied in r49198. Closing ticket.

Note: See TracTickets for help on using tickets.