Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#3929 closed defect (fixed)

tools/mk_cfgrpt.sh missing from tarball

Reported by: jreed Owned by: tmark
Priority: low Milestone: Kea0.9.2
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: 6 Internal?: no

Description

chmod: ./tools/mk_cfgrpt.sh: No such file or directory
./configure: ./tools/mk_cfgrpt.sh: not found

add to EXTRA_DIST

Subtickets

Change History (19)

comment:1 follow-up: Changed 2 years ago by jreed

since not noticed -- should have some test to verify the config.report was embedded too?

comment:2 Changed 2 years ago by jreed

Also why is src/bin/cfgrpt/config_report.cc in the repo and in the tarball? It contains irrelevant or wrong details.

comment:3 in reply to: ↑ 1 Changed 2 years ago by fdupont

Replying to jreed:

since not noticed -- should have some test to verify the config.report was embedded too?

=> both "-W" and the strings + sed described in guides.

comment:4 follow-up: Changed 2 years ago by jreed

Not in the repo. The Makefile.am should use nodist_libcfgrpt_la_SOURCES ("nodist"). EXTRA_DIST used for the tool.

A shell test could confirm the uname matches when running a tool with the switch.

comment:5 in reply to: ↑ 4 Changed 2 years ago by fdupont

Replying to jreed:

Not in the repo. The Makefile.am should use nodist_libcfgrpt_la_SOURCES ("nodist"). EXTRA_DIST used for the tool.

=> I agree.

A shell test could confirm the uname matches when running a tool with the switch.

=> I don't understand you idea. IMHO the best is you take the ticket.

comment:6 Changed 2 years ago by marcin

  • Milestone changed from Kea-proposed to Kea0.9.2
  • Priority changed from medium to low

Moving to 0.9.2 per Kea call on 07/08/2015

comment:7 Changed 2 years ago by wlodekwencel

  • Owner set to wlodekwencel
  • Status changed from new to assigned

comment:8 Changed 2 years ago by tmark

  • Owner changed from wlodekwencel to tmark

comment:9 follow-up: Changed 2 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 6

Moved cfgrpt directory from bin to lib since it creates a library
rather than an executable.

The source file, "config_report.cc" is now generated by cfgrpt/Makefile
rather than by configure. This allows it to be treated like any other
generated source file.

Added the pathname to "config.report" as a parameter to tools/mk_cfgrpt.sh.
This ensures we use the same file to generate config_report.cc as the file
we used in the rule for "config_report.cc" (see cfgrpt/Makefile.am)

Added simple unit test to cfgrpt.

It passes make distcheck under OS-X.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 2 years ago by fdupont

Replying to tmark:

Moved cfgrpt directory from bin to lib since it creates a library
rather than an executable.

=> PLEASE DON'T. IT IS NOT A SHARED LIBRARY.

The source file, "config_report.cc" is now generated by cfgrpt/Makefile
rather than by configure. This allows it to be treated like any other
generated source file.

=> note this adds a dependency on a not standard file. I am not sure it is a good idea.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 2 years ago by tmark

Replying to fdupont:

Replying to tmark:

Moved cfgrpt directory from bin to lib since it creates a library
rather than an executable.

=> PLEASE DON'T. IT IS NOT A SHARED LIBRARY.

It creates a library that is linked into virtually all of the executables that we create. It belongs in under lib, not bin.

The source file, "config_report.cc" is now generated by cfgrpt/Makefile
rather than by configure. This allows it to be treated like any other
generated source file.

=> note this adds a dependency on a not standard file. I am not sure it is a good idea.

By non-standard file I presume you mean "config.report". The simple truth is that the dependency does exist: config_report.cc is dependent on config.report. If we want the build to a: succeed b: be accurate then it should
recognize the dependency.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 2 years ago by fdupont

Replying to tmark:

Moved cfgrpt directory from bin to lib since it creates a library
rather than an executable.

=> PLEASE DON'T. IT IS NOT A SHARED LIBRARY.

It creates a library that is linked into virtually all of the executables that we create. It belongs in under lib, not bin.

=> first lib are for shared libraries only and it is not. Second we already had this discussion so this subject is supposed to be already closed.

=> note this adds a dependency on a not standard file. I am not sure it is a good idea.

By non-standard file I presume you mean "config.report". The simple truth is that the dependency does exist: config_report.cc is dependent on config.report.

=> not in the same extend: if someone removes the config.report too soon because (s)he doesn't what it is for the build will break. At the other side you can say if you want only to build shared libraries (aka src/lib directory) you had a wild dependency to address too.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 2 years ago by tmark

Replying to fdupont:

Replying to tmark:

Moved cfgrpt directory from bin to lib since it creates a library
rather than an executable.

=> PLEASE DON'T. IT IS NOT A SHARED LIBRARY.

It creates a library that is linked into virtually all of the executables that we create. It belongs in under lib, not bin.

=> first lib are for shared libraries only and it is not. Second we already had this discussion so this subject is supposed to be already closed.

No such discussion is reflected in the ticket nor was it documented in any code that I could find. What coding standard or guide states that lib is only for shared libs? To be accurate I think you mean libs that are distributed, not libs that are shared. One can just as easily insist that bin is only for executables, so maybe this thing belongs in src/misc.

=> note this adds a dependency on a not standard file. I am not sure it is a good idea.

By non-standard file I presume you mean "config.report". The simple truth is that the dependency does exist: config_report.cc is dependent on config.report.

=> not in the same extend: if someone removes the config.report too soon because (s)he doesn't what it is for the build will break. At the other side you can say if you want only to build shared libraries (aka src/lib directory) you had a wild dependency to address too.

I can't see why anyone would have a compelling reason to delete "config.report". If they are building from source it seems unlikely they would got to any lengths to remove it. To build, they must run configure, even if they only want to build the libraries. The libraries need <topdir>/config.h, so I don't really see the issue., I don't see a disadvantage to them also needing <topdir>/config.report. You can't build the src/lib without running configure, so I fail to see the issue.

A third opinion would be helpful.

comment:14 Changed 2 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:15 in reply to: ↑ 13 Changed 2 years ago by fdupont

Replying to tmark:

Replying to fdupont:

Replying to tmark:

Moved cfgrpt directory from bin to lib since it creates a library
rather than an executable.

=> PLEASE DON'T. IT IS NOT A SHARED LIBRARY.

It creates a library that is linked into virtually all of the executables that we create. It belongs in under lib, not bin.

=> first lib are for shared libraries only and it is not. Second we already had this discussion so this subject is supposed to be already closed.

No such discussion is reflected in the ticket nor was it documented in any code that I could find.

=> in this ticket? Of course it is not in this ticket but it is in #3513.

What coding standard or guide states that lib is only for shared libs?

=> the fact there were some status libs/archives in many (including other) places. Now if you believe the shlib name is better I agree but it is a bit late to change.

To be accurate I think you mean libs that are distributed, not libs that are shared.

=> it doesn't make a big difference as they are distributed by default as shared (formally you are right and there is a static-link config file).

One can just as easily insist that bin is only for executables, so maybe this thing belongs in src/misc.

=> cfgrpt is *only* for executables. BTW it is this argument I used in #3513 when this issue raised.

I can't see why anyone would have a compelling reason to delete "config.report".

=> just because it is not standard. BTW the config.h file IS standard at the opposite.

A third opinion would be helpful.

=> Shawn reviewed #3513.

comment:16 follow-up: Changed 2 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit c2c8a9087c5464c346c76295608b6a2baf544c7f

Addressing the issue of the "lib" directory:

Moved cfgrpt directory from bin to lib since it creates a library rather than an executable.

=> PLEASE DON'T. IT IS NOT A SHARED LIBRARY.

It creates a library that is linked into virtually all of the executables that we create. It belongs in under lib, not bin.

first lib are for shared libraries only and it is not. Second we already had this discussion so this subject is supposed to be already closed.

I side with Thomas here and "lib" is the right place. There needs to be a place for code that is shared between binaries: whether that code ends up in a shared library or an object library that is directly included in each executable is a fine distinction. Either way, the code is linked into more than one executable, so should be in a common place.

Out of interest, I looked up the section on installation directory variables in the GNU coding standards:

https://www.gnu.org/prep/standards/html_node/Directory-Variables.html

... where it says:

‘libdir’

The directory for object files and libraries of object code. Do not install executables here, they probably ought to go in $(libexecdir) instead. The value of libdir should normally be /usr/local/lib, but write it as $(exec_prefix)/lib. (If you are using Autoconf, write it as ‘@libdir@’.)

It says nothing about libdir being reserved exclusively for shared libraries. (OK, this refers to directories in the target system, but I think the principle should be the same - "lib" reserved for object files and object file libraries.)

As to the issue of "config.report":

I think that if someone starts messing with files in the source tree between the configure step and the build step, they shouldn't be too surprised if the build fails. So I don't think that the added dependency, although not standard, is an issue.


I've reviewed the files and have just one comment:

./Makefile.am
At some point in the future, the comment about the files included in EXTRA_DIST needs to be updated. But it's not important and can be ignored for now.

I think that the code can be merged as it is now.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 2 years ago by fdupont

Replying to stephen:

I side with Thomas here and "lib" is the right place.

=> so it is two (me and Shawn) against two... Note if we shall have the discussion each time there is something changed in the cfgrpt stuff (i.e., not follow #3513) I prefer to give up.

There needs to be a place for code that is shared between binaries: whether that code ends up in a shared library or an object library that is directly included in each executable is a fine distinction.

=> it is a fine distinction great enough to make the cfgrpt to break if someone doesn't understand it is not a shared library and "fix" it making it like other members of src/lib...

Either way, the code is linked into more than one executable, so should be in a common place.

=> bin was perfect.

Out of interest, I looked up the section on installation directory variables in the GNU coding standards:

=> IMHO not relevant because chgrpt is not installed (i.e., the place to not install it is out of interest :-).

comment:18 Changed 2 years ago by tmark

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

Changes merged with git# 223626338ab56b4404229ab72b47a8c2e59f1a6f

comment:19 in reply to: ↑ 17 Changed 2 years ago by sar

Replying to fdupont:

Replying to stephen:

I side with Thomas here and "lib" is the right place.

=> so it is two (me and Shawn) against two... Note if we shall have the discussion each time there is something changed in the cfgrpt stuff (i.e., not follow #3513) I prefer to give up.

For the record, I prefer this being in lib - I was willing to accept it elsewhere but agree with the others that lib is better.

Note: See TracTickets for help on using tickets.