add dependencies options and -fcopybook-deps #109

lefessan · 2023-08-15T19:44:06Z

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 · 2023-08-15T19:49:04Z

Codecov Report

Attention: Patch coverage is 85.46512% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 65.90%. Comparing base (89c45a3) to head (75646da).

Files	Patch %	Lines
cobc/cobc.c	86.46%	6 Missing and 12 partials ⚠️
cobc/error.c	76.47%	2 Missing and 2 partials ⚠️
cobc/pplex.l	86.66%	1 Missing and 1 partial ⚠️
cobc/typeck.c	0.00%	0 Missing and 1 partial ⚠️

❗ 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              65.87%   65.90%   +0.03%     
=====================================================
  Files                     32       32              
  Lines                  59483    59622     +139     
  Branches               15709    15747      +38     
=====================================================
+ Hits                   39182    39292     +110     
- Misses                 14282    14310      +28     
- Partials                6019     6020       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

GitMensch · 2023-08-15T21:02:30Z

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

cobc/pplex.l

lefessan · 2023-08-16T17:00:56Z

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 · 2023-08-16T18:01:33Z

@GitMensch I have a completely unrelated question ; I have a PR on the manual in OCamlPro/gnucobol-docs#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)?

tests/testsuite.src/used_binaries.at

GitMensch · 2023-10-20T05:20:52Z

tests/testsuite.src/used_binaries.at

+AT_CHECK([$COMPILE -MD  prog.cob])
+
+AT_CHECK([cat prog.d], [0],
+[prog.o: \


that test would fail when COB_OBJECT_EXT != o

That still fails with a compiler using ".obj" or else - no? Note: $COB_OBJECT_EXT is available in the testuite so a $SED operation should be possible.

Should work now even with .obj, using $SED on compiler output...

tests/testsuite.src/used_binaries.at

GitMensch · 2024-02-08T11:17:15Z

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 · 2024-02-20T13:38:09Z

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

cobc/ChangeLog

GitMensch · 2024-03-12T17:01:59Z

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 · 2024-03-15T13:36:32Z

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.

GitMensch

Nearly finished! Please see comments.

GitMensch · 2024-03-15T14:35:57Z

cobc/flag.def

@@ -134,6 +134,9 @@ CB_FLAG (cb_flag_stack_extended, 1, "stack-extended",
 CB_FLAG_ON (cb_flag_fast_compare, 0, "fast-compare",
 	_("  -fno-fast-compare     disables inline comparisions"))

+CB_FLAG (cb_flag_copybook_deps, 0, "copybook-deps",
+	_("  -fcopybook-deps       output copybook names as dependencies"))


The M... options should already have some docs in gnucobol.texi and may should be extended (it is fine to reference GCC documentation for detail, it is also fine to copy its documentation over - both documents have the same license and copyright owner).
The option to getting a grasp how/why one should use them in the manual would be very useful.

The -fcopybook-deps is something new and "special", there definitely should be at least one sentence in the manual on which case/why to use that.

The currently missing NEWS entry can then also reference the manual for details. :-)

cobc/help.c

cobc/flag.def

cobc/error.c

cobc/ChangeLog

GitMensch · 2024-03-15T14:49:04Z

tests/testsuite.src/used_binaries.at

+  AT_CHECK([$GREP 'sub\\copy\\PROC.cpy:'     prog.d], [0], [ignore])
+  AT_CHECK([$GREP 'sub\\PROCE.cpy:'          prog.d], [0], [ignore])
+], [
+  AT_CHECK([$GREP ' sub/copy/PROC.cpy \\'    prog.d], [0], [ignore])


Why do we still need that? I've thought the point of slashify is to not need that any more?

Indeed, it's probably not useful anymore, but without a Windows computer, it's painful to modify all these tests and then wait for Github CI for feedback...

I see the pain, but please check that with either Github CI (that's still much faster than Appveyor) or otherwise, when adding the cob_slashify - which should be in a separate PR. Instead this PR should use the same approach that's used here or a simple tr of the test data for output.

GitMensch · 2024-03-15T14:50:15Z

tests/testsuite.src/used_binaries.at

+AT_CHECK([$COMPILE -MD  prog.cob])
+
+AT_CHECK([cat prog.d], [0],
+[prog.o: \


That still fails with a compiler using ".obj" or else - no? Note: $COB_OBJECT_EXT is available in the testuite so a $SED operation should be possible.

GitMensch · 2024-03-15T14:59:24Z

tests/testsuite.src/used_binaries.at

       PROCEDURE        DIVISION.
-           CALL "f" USING "Hello" BY VALUE long RETURNING NOTHING.


This has, by design a BY VALUE long in there and ignores the compiler output; is there a reason to change that?
What is that "rename" stuff about?

So, under Windows, the function has to be available. f was not a real function, so it was failing with an "undefined function 'f'". I solved it by calling an existing function (rename is part of stdio.h, which is not included by default in common.h). My understanding was that the test was mostly testing the arity of the function, not the exact types, so I chose a function with two char* because I couldn't find an existing function with the same prototype as former f.

rechecking:

The first test is a simple non-static call, so no problem.
The second one does a static call, and that may need to be resolved to not raise an error, so yes, it would be reasonable to adjust this - but please put that to another (separate! PR) and leave this out here.

GitMensch · 2024-03-15T15:02:33Z

tests/testsuite.src/used_binaries.at

+AT_CHECK([echo "$PWD/prog.cob:7: error: CRUD.CPY: No such file or directory" > expected.output])
+AT_CHECK([echo "$PWD/prog.cob:6: warning: numeric value is expected" >> expected.output])
+AT_CHECK([echo "$PWD/prog.cob:14: warning: ignoring redundant ." >> expected.output])
+AT_CAPTURE_FILE([expected.output])
+
+AT_CHECK([[cat compiler.output | tr '[:upper:]' '[:lower:]' | tr -d ':/\\' > compiler.output2]])
+AT_CHECK([[cat expected.output | tr '[:upper:]' '[:lower:]' | tr -d ':/\\' > expected.output2]])
+AT_CHECK([diff compiler.output2 expected.output2])


Can we use a HEREDOC here instead of AT_CHECK?

I am not familar with HEREDOC notations. This version is not the most elegant one, but it does the job :-)

This is all unrelated to the actual PR changes, please move this out to a separate PR for later review.

GitMensch · 2024-03-15T15:04:22Z

tests/testsuite.src/used_binaries.at

-AT_CHECK([TMPDIR="" TMP="notthere" TEMP="" $COMPILE prog.cob], [0], [],
-[libcob: warning: Temporary directory TMP is invalid, adjust TMPDIR!
+AT_CHECK([TMPDIR="" TMP="notthere" TEMP="" $COMPILE prog.cob 2> compiler.output], [0], [],
+[],[
+    # On Windows, we get a failure from gcc, so the binary is not created, the stderr is mixed and exit code is 1.
+    AT_CHECK([grep libcob: compiler.output], [0], [libcob: warning: Temporary directory TMP is invalid, adjust TMPDIR!
 ])
-AT_CHECK([$COBCRUN_DIRECT ./prog], [0], [OK], [])
-AT_CHECK([TMPDIR="" TMP="" TEMP="./prog.cob" $COMPILE prog.cob], [0], [],
-[libcob: warning: Temporary directory TEMP is invalid, adjust TMPDIR!
+  ],
+  [
+    AT_CHECK([cat compiler.output], [0], [libcob: warning: Temporary directory TMP is invalid, adjust TMPDIR!
+])
+    AT_CHECK([$COBCRUN_DIRECT ./prog], [0], [OK], [])
+  ])
+
+AT_CHECK([TMPDIR="" TMP="" TEMP="./prog.cob" $COMPILE prog.cob 2> compiler.output], [0], [],
+[],[
+    AT_CHECK([grep libcob: compiler.output], [0], [libcob: warning: Temporary directory TEMP is invalid, adjust TMPDIR!
+])
+  ],
+  [
+    AT_CHECK([cat compiler.output], [0], [libcob: warning: Temporary directory TEMP is invalid, adjust TMPDIR!
 ])
+  ])
+


that's an ... interesting approach, but please move that out to a separate PR

The problem with moving that elsewhere is that the test will keep failing under Windows, which means that I might make changes in this PR and not notice that the failures under Windows have changed. I would prefer to have a working CI under Windows and find another fix in another PR later...

lefessan · 2024-03-17T15:33:05Z

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 · 2024-03-17T15:34:02Z

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

Flags following gcc options: -M to output deps only, -MD to output deps while compiling (in .d files), -MP to output phony targets, -MG to keep missing copybooks, -MQ <target> to Makefile-quote target Additional flags specific to GnuCOBOL: -fcopybook-deps outputs only copybook names instead of file paths. -fcopybook-deps also forces -E.

lefessan · 2024-03-19T08:25:10Z

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

GitMensch

Nah, "review pending" - I haven't clicked on send.

But this let me recheck another time now and after thinking "what do we want to achieve" I concluded that it is not the Win32 test working fine afterwards.

I therefore added some review comments asking for moving those changes out to separate PR(s) to be reviewed separately (and definitely later), allowing to merge this R hopefully next week.

Thank you and Sorry for the postponed review!

GitMensch · 2024-03-19T15:28:24Z

cobc/cobc.c

-static char		*cb_depend_target;	/* -MT <target>... */

+static char		*cb_depend_target;	/* -MT <target>... */
+static const char       *cb_depend_filename;    /* -MF <file> */


those are all pointer to chars, so the old space line after seems more reasonable (but it's enough to handle that on commit and no problem if you forget, other than people possibly stumbling over this later).

Actually, because they don't follow the "size" couple that the others have, I'd suggest to moving them up front with an extra space line after.

GitMensch · 2024-03-19T15:30:36Z

cobc/cobc.c

+static int string_is_dash (const char*s){
+	return ( strcmp (s, COB_DASH) == 0 );
+}
+
+static void add_depend_target (const char* s){


minor formatting:

first the type

next line the name and its arguments

next line the starting {

GitMensch · 2024-03-19T15:31:57Z

cobc/cobc.c

@@ -3001,6 +3018,74 @@ remove_trailing_slash (char *data)
 	}
 }

+static int string_is_dash (const char*s){


please force-inline this function

GitMensch · 2024-04-20T19:28:03Z

tests/testsuite.src/used_binaries.at

+
+AT_CLEANUP
+
+AT_SETUP([output dependencies])


please add an extra empty line before that

GitMensch · 2024-04-20T19:45:48Z

tests/testsuite.src/used_binaries.at

+  AT_CHECK([$GREP 'sub\\copy\\PROC.cpy:'     prog.d], [0], [ignore])
+  AT_CHECK([$GREP 'sub\\PROCE.cpy:'          prog.d], [0], [ignore])
+], [
+  AT_CHECK([$GREP ' sub/copy/PROC.cpy \\'    prog.d], [0], [ignore])


I see the pain, but please check that with either Github CI (that's still much faster than Appveyor) or otherwise, when adding the cob_slashify - which should be in a separate PR. Instead this PR should use the same approach that's used here or a simple tr of the test data for output.

GitMensch · 2024-04-20T19:58:13Z

tests/testsuite.src/used_binaries.at

       PROCEDURE        DIVISION.
-           CALL "f" USING "Hello" BY VALUE long RETURNING NOTHING.


rechecking:

The first test is a simple non-static call, so no problem.
The second one does a static call, and that may need to be resolved to not raise an error, so yes, it would be reasonable to adjust this - but please put that to another (separate! PR) and leave this out here.

GitMensch · 2024-04-20T20:02:05Z

tests/testsuite.src/used_binaries.at

+AT_CHECK([echo "$PWD/prog.cob:7: error: CRUD.CPY: No such file or directory" > expected.output])
+AT_CHECK([echo "$PWD/prog.cob:6: warning: numeric value is expected" >> expected.output])
+AT_CHECK([echo "$PWD/prog.cob:14: warning: ignoring redundant ." >> expected.output])
+AT_CAPTURE_FILE([expected.output])
+
+AT_CHECK([[cat compiler.output | tr '[:upper:]' '[:lower:]' | tr -d ':/\\' > compiler.output2]])
+AT_CHECK([[cat expected.output | tr '[:upper:]' '[:lower:]' | tr -d ':/\\' > expected.output2]])
+AT_CHECK([diff compiler.output2 expected.output2])


This is all unrelated to the actual PR changes, please move this out to a separate PR for later review.

GitMensch · 2024-04-20T20:02:41Z

doc/gnucobol.texi

That's well done. Please add a ChangeLog entry for it.

GitMensch · 2024-04-20T20:06:24Z

cobc/error.c

+the test of outputs, as they will be similar on Unix and Windows.
+*/
+char *
+cobc_slashify (const char *src)


I see. But cob_slashify() and its related testsuite changes are far from the original PR. Please move that out to a separate one, then revert the related parts in this one.

GitMensch · 2024-04-20T20:09:06Z

.github/workflows/windows.yml

      - name: check
-        continue-on-error: true


What about moving this to two separate steps? One "check" with continue-on-error and another test that should never fail? After all these run as separate checks for win32 in any case.

lefessan force-pushed the z-2023-07-15-deps branch 2 times, most recently from b79b1e2 to e3c6cd9 Compare August 16, 2023 16:55

GitMensch reviewed Aug 16, 2023

View reviewed changes

cobc/pplex.l Show resolved Hide resolved

lefessan force-pushed the z-2023-07-15-deps branch 4 times, most recently from 5cfc276 to 9aeb62c Compare August 16, 2023 17:20

lefessan force-pushed the z-2023-07-15-deps branch from 9aeb62c to e2ed1a0 Compare August 18, 2023 19:54

GitMensch requested changes Oct 20, 2023

View reviewed changes

lefessan force-pushed the z-2023-07-15-deps branch from 029378b to 0d408ba Compare March 12, 2024 14:21

GitMensch reviewed Mar 12, 2024

View reviewed changes

cobc/ChangeLog Outdated Show resolved Hide resolved

lefessan force-pushed the z-2023-07-15-deps branch 2 times, most recently from 136592b to 8dc9eed Compare March 15, 2024 09:22

lefessan changed the title ~~add new flags -foneline-deps and -fcopybook-deps to output dependencies~~ add dependencies options and -fcopybook-deps to output dependencies Mar 15, 2024

lefessan changed the title ~~add dependencies options and -fcopybook-deps to output dependencies~~ add dependencies options and -fcopybook-deps Mar 15, 2024

lefessan force-pushed the z-2023-07-15-deps branch from bea01c0 to 98c078f Compare March 15, 2024 13:12

GitMensch requested changes Mar 15, 2024

View reviewed changes

lefessan force-pushed the z-2023-07-15-deps branch from 98c078f to d7be103 Compare March 17, 2024 15:27

lefessan force-pushed the z-2023-07-15-deps branch 2 times, most recently from 7cf30b0 to 68c30e6 Compare March 17, 2024 15:54

lefessan force-pushed the z-2023-07-15-deps branch from 68c30e6 to 75646da Compare March 17, 2024 15:58

GitMensch requested changes Apr 20, 2024

View reviewed changes

GitMensch mentioned this pull request Apr 23, 2024

Add/update Windows workflows #140

Merged

		PROCEDURE DIVISION.
		CALL "f" USING "Hello" BY VALUE long RETURNING NOTHING.


		AT_CLEANUP

		AT_SETUP([output dependencies])

add dependencies options and -fcopybook-deps #109

Are you sure you want to change the base?

add dependencies options and -fcopybook-deps #109

Conversation

lefessan commented Aug 15, 2023 • edited Loading

codecov-commenter commented Aug 15, 2023 • edited Loading

Codecov Report

GitMensch commented Aug 15, 2023

lefessan commented Aug 16, 2023

lefessan commented Aug 16, 2023 • edited Loading

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitMensch commented Feb 8, 2024

GitMensch commented Feb 20, 2024

GitMensch commented Mar 12, 2024

lefessan commented Mar 15, 2024

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lefessan commented Mar 17, 2024

lefessan commented Mar 17, 2024

lefessan commented Mar 19, 2024

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lefessan commented Aug 15, 2023 •

edited

Loading

codecov-commenter commented Aug 15, 2023 •

edited

Loading

lefessan commented Aug 16, 2023 •

edited

Loading