Ticket #1130 (closed bug: fixed)

Opened 5 years ago

Last modified 4 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 5 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 5 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 5 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 5 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 4 years ago by jkeenan

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

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 4 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.