add dependencies options and -fcopybook-deps

[lefessan]

trafficstars

• Implements dependencies options similar to gcc
• -fcopybook-deps outputs only copybook names instead of file paths. -fcopybook-deps also forces -E -foneline-deps -MT=copybooks, disables errors on missing copybooks and remove output on stdout.

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 95.39474% with 7 lines in your changes missing coverage. Please review.

Project coverage is 77.56%. Comparing base (2c000f5) to head (88e292a).

Files with missing lines	Patch %	Lines
cobc/cobc.c	94.65%	7 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                 Coverage Diff                 @@
##           gcos4gnucobol-3.x     #109    +/-   ##
===================================================
  Coverage              77.56%   77.56%            
===================================================
  Files                     33       33            
  Lines                  60230    60349   +119     
  Branches               15834    15869    +35     
===================================================
+ Hits                   46717    46810    +93     
- Misses                 13513    13539    +26     

Flag	Coverage Δ
	77.56% <95.39%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[GitMensch]

As testcases are missing I cannot get a grasp what this is about....

We definitely should provide a way to "only" calculate the dependencies and in this case the paths are not available so we can only output the names, which may be the idea behind that.

Not sure why we should have a "only on one line".

Adjustments to -MT and friends are useful and those were added back with an explicit note that there will be changes - but those should mimic what GCC does with them so please provide sample code and output of a C program with #include <something.h> and #include <else/something.h> and #include "other/something.h" along with some different -I and -M... options and add test cases that have the same scenario for cobc - making the later mimic the first is the way to go. Only when we have that we should inspect your scenario to check if that's what you need or what is missing.

And yes: if you could tackle the -M... options, that would be quite good (but those changes should mimic recent GCC, both from their options and from their results).

[lefessan]

I added a test in used_binaries.at... and most arguments used by gcc that make sense for GnuCOBOL. I don't know which changes are being done in gcc, but I can import them accordingly.

For -fcopybook-deps, the idea is to output dependencies for other tools than make, in a very simple format and without file path resolution. For example, using this output, it is possible to create a tool that will use it to know the copybooks of a source file, and then search for them by scanning directories, and finally call cobc again with an automatically generated list of -I <dir>.

[lefessan]

@GitMensch I have a completely unrelated question ; I have a PR on the manual in https://github.com/OCamlPro/gnucobol-docs/pull/2 . Is there another maintainer for that part, or are you also in charge ?

[GitMensch]

Should -MM and -MMD be added, ignoring the copybooks in COB_COPY_DIR (currently extfh and screenio)?

Shouldn't the dependency options be included in gnucobol.texi, possibly copying GCCs docs verbatim? This would also allow to note in NEWS that we adjusted these options (referencing the 3.2 note about the future change) while pointing to the manual for the details on the dependency generation.

In any case this should be split to two commits for SVN - one that adjusts the "standard" dependency options and a second one for the new flags. I'd like to review them separately, can you please split the PR into two (obviously the new flags would not base on the original target but on the deps branch)?

Friendly ping @lefessan

[GitMensch]

@lefessan I've did a quick browser-only merge, can you please review, adjust as needed and check the review comment above?

[GitMensch]

Thanks for the rebase. As guessed before, noted in the review and now seen in the CI: we have issues at least on Win32 as follows:

#                             -*- compilation -*-
28. used_binaries.at:1119: testing output dependencies ...
../../tests/used_binaries.at:1138: mkdir -p sub
../../tests/used_binaries.at:1144: $COMPILE_ONLY prog.cob
../../tests/used_binaries.at:1146: $COMPILE_ONLY -M prog.cob prog.cob
--- -	2024-03-12 14:25:51.094447400 +0000
+++ /d/a/gnucobol/gnucobol/_build/tests/testsuite.dir/at-groups/28/stdout	2024-03-12 14:25:51.042599800 +0000
@@ -2,12 +2,12 @@
  prog.cob \
  COPY1.CPY \
  COPY2.CPY \
- sub/COPY3.CPY
+ sub\COPY3.CPY
 
 prog.o: \
  prog.cob \
  COPY1.CPY \
  COPY2.CPY \
- sub/COPY3.CPY
+ sub\COPY3.CPY

I think all make implementations support / paths, in this case we may use these here; otherwise the test may get a conditional to change the expected result for Win32 to backslash or we go with adjusting the output of (.)\(.) to $1/$2.

[lefessan]

So, in this version, I am using the COB_IS_RUNNING_IN_TESTMODE variable to replace backslashes by slashes in the compiler output, so that tests have the same results on Unix and Windows.

[lefessan]

Should -MM and -MMD be added, ignoring the copybooks in COB_COPY_DIR (currently extfh and screenio)?

I don't think it is worth it. Though they are easy to implement (by modifying the cb_copy_find_file in pplex.l to set a flag when using the COB_COPY_DIR entry of cb_include_list), I don't see any reason why the external tool that would use the dependencies would need to remove system dependencies, or wouldn't be able to do it by itself. Adding low value features makes the code harder to maintain for little benefit.

[lefessan]

This version has a NEWS section and a new section in gnucobol.texi for dependency options.

[lefessan]

@GitMensch I think I have replied and/or fixed all your points, is it for merge ?

[ddeclerck]

Took care of most changes ; as far as I can tell the only thing that remains to do is splitting the unrelated testsuite changes and cobc_slashify to another PR, right ?

[ddeclerck]

@GitMensch I think I took all remarks into account. Is this ok for you ?

[ddeclerck]

Just a question: -MMD and -isystem are left for a later PR,. right?

Not sure ; quoting Fabrice's reply (https://github.com/OCamlPro/gnucobol/pull/109#issuecomment-2002510812):

Should -MM and -MMD be added, ignoring the copybooks in COB_COPY_DIR (currently extfh and screenio)?

I don't think it is worth it. Though they are easy to implement (by modifying the cb_copy_find_file in pplex.l to set a flag when using the COB_COPY_DIR entry of cb_include_list), I don't see any reason why the external tool that would use the dependencies would need to remove system dependencies, or wouldn't be able to do it by itself. Adding low value features makes the code harder to maintain for little benefit.

[GitMensch]

I agree in general (and COB_COPY_DIR would only be applied for the inbuilt one, because the env var is used by users for their copybooks as well).

If added, then this makes only sense with -isystem (= places for copybooks a user don't want to see), but we can leave that out until there is someone that would like to use it. As noted in the review, I think they're one leftover var related to that.

[GitMensch]

Years ago I'd have said yes, but it is best to prevent function-like macros. I'm fine with an inline function.

Am 22. September 2024 12:33:39 MESZ schrieb OCP David Declerck @.***>:

@ddeclerck commented on this pull request.

+static int +string_is_dash (const char *s) +{

• return (strcmp (s, COB_DASH) == 0); +}

I like the idea of a descriptive name such as string_is_dash for this (plus it's somewhat "easier" to write). Would a macro be acceptable for this ? Otherwise I'll just reverse this change.

-- Reply to this email directly or view it on GitHub: https://github.com/OCamlPro/gnucobol/pull/109#discussion_r1770490319 You are receiving this because you were mentioned.

Message ID: @.***>

[GitMensch]

Yes.

Am 22. September 2024 13:33:17 MESZ schrieb OCP David Declerck @.***>:

@ddeclerck commented on this pull request.

+AT_CHECK([mkdir -p sub]) +AT_DATA([COPY1.CPY], []) +AT_DATA([COPY2.CPY], [ DISPLAY "Hello". +]) +AT_DATA([sub/COPY3.CPY], [])

Seems this is because pp_text_list_add does not check for duplicates. This function was present before this patch and seems to be only used to build the dependency list: is it okay to change it so that it check for duplicates unconditionally ?

-- Reply to this email directly or view it on GitHub: https://github.com/OCamlPro/gnucobol/pull/109#discussion_r1770529755 You are receiving this because you were mentioned.

Message ID: @.***>

[GitMensch]

From the GCC docs the following may be not "correct" yet:

Passing -M to the driver implies -E, and suppresses warnings with an implicit -w.

Note that we can, of course, should be able to copy most of the GCC docs about those options to gnucobol.texi.

Would there be any reason to support DEPENDENCIES_OUTPUT env var?

[GitMensch]

I'm quite sure that there were also recent adjustments from you :-) But you can tackle this changelog change directly when committing upstream.

[ddeclerck]

I'm quite sure that there were also recent adjustments from you

Not really, just minor changes: allow duplicates in pplex.l (I added to the ChangeLog), moving the flag from flag.def to cobc.c (I adjusted the filenames in the ChangeLog entry), some whitespaces fixes (adding a space when I found ){ glued together), and a two or three const added (to the new code, not to existing code).

Unless I'm missing something (I'm working on 10 things at the same time, I'm kinda lost)

[GitMensch]

then let's only out the duplicate stuff in pplex.l to recent changelog entry from you before commit (and possibly leave flags.def in upstream as-is)

[ddeclerck]

then let's only out the duplicate stuff in pplex.l to recent changelog entry from you before commit (and possibly leave flags.def in upstream as-is)

Ah, I had added that to Fabrice's entry from 03/2024. It's only a 3-line patch, I'm okay with not taking the credit for that 😂

[GitMensch]

I'd like to keep the Changelog including time entries relative clean, that's not (only) about attribution (also a the date is quite interesting) eeeeeeeeeI see from https://github.com/OCamlPro/gnucobol/compare/1a2949b4df2a55b7bca16f13a91c53a2e3807a9d..236c8b2c21dbaf470250d48bd2a014be4b10aaf1 that the adjustment is mostly testcases.

[ddeclerck]

Didn't notice this was granted the "Ready for SVN" tag 😅. Rebasing before upstreaming.

[GitMensch]

The most simple invocation needs to be adjusted, as this does not behave as GCC (and that's what for example IBM's cob2 follows)

$> touch  def.h
$> echo '#include "def.h"' > test.c
$> gcc -M test.c
 test.o: test.c /usr/include/stdc-predef.h def.h
$> ls -t | head
test.c def.h

$> touch def.cob
$> echo '       COPY "def.cob".' > test.cob
$> cobc -M test.cob
test.o: \
 test.cob \
 def.cob
$> ls -t | head
test.i test.cob def.cob

The format is minimal different but "logically (for make) identical", so I don't mind. The single part that needs an adjustment is that test.i should only be stored if --save-temps is used.

And for the cobc extension that creates the copybook dependencies only -fcopybook-deps, there is an issue if a file has no dependencies:

$> touch def.cob other.cob
$> cobc -fcopybook-deps def.cob other.cob
def.o:other.o$>    # missing line breaks

Also we should note in NEWS that the 3.2 way (necessary to specify both -MT and -MF) is not supported any more, to reach the same you need to add -MD now (create while compiling) or -M (only preprocess and generate dependency file), while -MT is optional and if -MF is not given the output goes to stdout.

[GitMensch]

tackled by #223 and now upstream