Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#3911 closed enhancement (complete)

need a guideline about libraries in Makefile.am files

Reported by: fdupont Owned by: fdupont
Priority: high Milestone: Kea1.0-beta
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Today there is no coherent policy about the dependencies for building a library in Makefile.am files.
Note as we use libtool there is nearly no strong requirements because libtool computes the dependency closure and saves it in the .la file.
So the choice which should be documented (in coding style?) and followed are:

  • minimal set: puts only the required dependencies
  • put only direct dependencies
  • put all dependencies (note we have it in the win32 branch).

We should decide the order too. IMHO the best order is the opposite of the build one, i.e., exceptions should be last.

There is nothing urgent but I'd like to get this agreed and applied before the versioning (aka #3771) work resumes.

Subtickets

Change History (9)

comment:1 Changed 4 years ago by fdupont

After some thinking I got a proposal:

  • put all dependencies, even indirect, in Makefile.am.
  • use the reversed built order (e.g., in most cases exceptions library will be the last)
  • add at the end external dependencies

The idea is:

  • not rely on libtool to compute the dependency closure
  • avoid backward dependency
  • use the dependency list as documentation (i.e., dependencies will be clear at the first sight).

Comments?

comment:2 Changed 4 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

Proposal approved by Stephen in the team list.

comment:3 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0
  • Priority changed from low to medium

Per team meeting on 8/12 move to 1.0 Med

comment:4 Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

The branch trac3911a is ready for review. Static link should work at the exception of #4004. Tested on OS X and 2 different Linuxes.
Note I don't remember if the guideline (all dependencies in the reverse src/lib directory order) is already in the wiki (if not I'll do it).
Another detail: for convenience archives the dependencies should be kept small because they are useless (i.e., not needed for linking) but some libtool's add extra/spurious libraries using the .la files (e.g., util/io linked each time util/unitests was referenced for run_all()).

comment:5 Changed 4 years ago by fdupont

  • Priority changed from medium to high

Got with #4009 some cases where libtool didn't forward dependencies and one where the LDADD order was critical. So boosting this ticket priority!

comment:6 Changed 4 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit 03681966ef234d1a3f667f7424507a9bf627a978

Only one trivial issue:

src/lib/hooks/tests/hooks_manager_unittest.cc
src/lib/hooks/tests/library_manager_collection_unittest.cc
Indentation in the method "dummy" is incorrect.

(I also got an error running the tests after a static link, but I notice that #4004 has been opened for that.)

I think it is ready to merge. It will need a ChangeLong entry, so how about:

Updated Makefiles to ensure that all required dynamic libraries
are included in the link command line as some systems are unable
handle implied library dependencies.

comment:8 Changed 4 years ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Removed the spurious tabs and merged with the proposed ChangeLog. As it can have unexpected side effect I launched Jenkins...
Now I'll update the guidelines (got some comments about this too).
Closing

comment:9 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.